Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix several compiler warnings #336

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Fix several compiler warnings #336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ionc/ion_reader_text.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ iERR _ion_reader_text_step_out(ION_READER *preader)
break;
default: {
char error_message[ION_ERROR_MESSAGE_MAX_LENGTH];
snprintf(error_message, ION_ERROR_MESSAGE_MAX_LENGTH, "Unable to step out of unrecognized container type %s", text->_current_container);
snprintf(error_message, ION_ERROR_MESSAGE_MAX_LENGTH, "Unable to step out of unrecognized container type %p", text->_current_container);
FAILWITHMSG(IERR_INVALID_STATE, error_message);
}
}
Expand Down Expand Up @@ -1231,7 +1231,7 @@ iERR _ion_reader_text_get_value_position(ION_READER *preader, int64_t *p_offset,
if (preader->_eof) {
offset = -1;
line = -1;
p_col_offset = -1;
p_col_offset = (int32_t *)-1;
}
else {
if (text->_annotation_start >= 0) {
Expand Down
18 changes: 11 additions & 7 deletions ionc/ion_stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@
#define READ read
#endif

// Helpers for storing int file descriptors in ION_STREAM's FILE* _fp field.
#define FD_TO_FILEP(x) ((FILE *)(size_t)(x))
#define FILEP_TO_FD(x) ((int)(size_t)(x))



//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -343,7 +347,7 @@ iERR ion_stream_open_fd_in( int fd_in, ION_STREAM **pp_stream )
}

IONCHECK(_ion_stream_open_helper(flags, g_Ion_Stream_Default_Page_Size, &stream));
stream->_fp = (FILE *)fd_in;
stream->_fp = FD_TO_FILEP(fd_in);
IONCHECK(_ion_stream_fetch_position(stream, 0));

*pp_stream = stream;
Expand All @@ -366,7 +370,7 @@ iERR ion_stream_open_fd_out( int fd_out, ION_STREAM **pp_stream )
}

IONCHECK(_ion_stream_open_helper(flags, g_Ion_Stream_Default_Page_Size, &stream));
stream->_fp = (FILE *)fd_out;
stream->_fp = FD_TO_FILEP(fd_out);
IONCHECK(_ion_stream_fetch_position(stream, 0));

*pp_stream = stream;
Expand All @@ -389,7 +393,7 @@ iERR ion_stream_open_fd_rw( int fd, BOOL cache_all, ION_STREAM **pp_stream )
}

IONCHECK(_ion_stream_open_helper(flags, g_Ion_Stream_Default_Page_Size, &stream));
stream->_fp = (FILE *)fd;
stream->_fp = FD_TO_FILEP(fd);
IONCHECK(_ion_stream_fetch_position(stream, 0));

*pp_stream = stream;
Expand Down Expand Up @@ -1213,7 +1217,7 @@ iERR _ion_stream_flush_helper(ION_STREAM *stream)
}
}
else if (_ion_stream_is_fd_backed(stream)) {
written = (SIZE)WRITE( (int)stream->_fp, stream->_dirty_start, stream->_dirty_length );
written = (SIZE)WRITE( FILEP_TO_FD(stream->_fp), stream->_dirty_start, stream->_dirty_length );
if (written != stream->_dirty_length) {
FAILWITH( IERR_WRITE_ERROR );
}
Expand Down Expand Up @@ -1605,7 +1609,7 @@ iERR _ion_stream_fseek( ION_STREAM *stream, POSITION target_position )
// short cut when we have a random access file backing the stream
if (_ion_stream_is_fd_backed(stream)) {
// TODO : should we validate this cast to long somehow?
if (LSEEK((int)stream->_fp, (long)target_position, SEEK_SET)) {
if (LSEEK(FILEP_TO_FD(stream->_fp), (long)target_position, SEEK_SET)) {
FAILWITH(IERR_SEEK_ERROR);
}
}
Expand Down Expand Up @@ -1840,7 +1844,7 @@ iERR _ion_stream_fread( ION_STREAM *stream, BYTE *dst, BYTE *end, SIZE *p_bytes_
//
local_bytes_read = (SIZE)(end - dst);
if (_ion_stream_is_fd_backed(stream)) {
bytes_read = (SIZE)READ((int)stream->_fp, dst, local_bytes_read);
bytes_read = (SIZE)READ(FILEP_TO_FD(stream->_fp), dst, local_bytes_read);
if (bytes_read < 0) {
bytes_read = READ_ERROR_LENGTH;
}
Expand Down Expand Up @@ -1999,7 +2003,7 @@ void _ion_stream_page_release(ION_STREAM_PAGED *paged, ION_PAGE *page )
page_id = page->_page_id;
test_page = _ion_index_find(&(paged->_index), &page_id);
if (test_page == page) {
_ion_index_delete(&(paged->_index), &page_id, &test_page);
_ion_index_delete(&(paged->_index), &page_id, (void**)&test_page);
ASSERT(test_page == page);
}

Expand Down
4 changes: 2 additions & 2 deletions test/test_ion_binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ TEST(IonBinaryInt, ReaderRejectsNegativeZeroMixedIntTwoByte) {
test_ion_binary_write_from_reader_rejects_negative_zero_int((BYTE *)"\xE0\x01\x00\xEA\x31\x00", 6);
}

void test_ion_binary_reader_threshold_for_int64_as_big_int(BYTE *data, size_t len, char *actual_value) {
void test_ion_binary_reader_threshold_for_int64_as_big_int(BYTE *data, size_t len, const char *actual_value) {
hREADER reader;
ION_TYPE type;
int64_t value;
Expand Down Expand Up @@ -478,7 +478,7 @@ TEST(IonBinaryReader, ReaderNegativeThresholdForInt64) {
// -2 ** 64
test_ion_binary_reader_threshold_for_int64_as_big_int((BYTE *)"\xE0\x01\x00\xEA\x38\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF", 13, "-18446744073709551615");
// -2 ** 63 fits as two's complement representation
test_ion_binary_reader_threshold_for_int64_as_int64((BYTE *)"\xE0\x01\x00\xEA\x38\x80\x00\x00\x00\x00\x00\x00\x00", 13, -9223372036854775808);
test_ion_binary_reader_threshold_for_int64_as_int64((BYTE *)"\xE0\x01\x00\xEA\x38\x80\x00\x00\x00\x00\x00\x00\x00", 13, 0x8000000000000000 /* -9223372036854775808 */);
}

void test_ion_binary_reader_requires_timestamp_fraction_less_than_one(BYTE *data, size_t len) {
Expand Down
12 changes: 6 additions & 6 deletions test/test_ion_stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,23 +164,23 @@ TEST(IonStream, WriteToUserStream) {

ION_ASSERT_OK(ion_writer_start_container(writer, tid_STRUCT));
ION_STRING fieldNameString;
ion_string_assign_cstr(&fieldNameString, "str_col1", strlen("str_col1"));
ion_string_assign_cstr(&fieldNameString, (char *)"str_col1", strlen("str_col1"));
ION_ASSERT_OK(ion_writer_write_field_name(writer, &fieldNameString));

ION_STRING value_string;
ion_string_assign_cstr(&value_string, "str_val1", strlen("str_val1"));
ion_string_assign_cstr(&value_string, (char *)"str_val1", strlen("str_val1"));
ION_ASSERT_OK(ion_writer_write_string(writer, &value_string));

ion_string_assign_cstr(&fieldNameString, "str_col2", strlen("str_col2"));
ion_string_assign_cstr(&fieldNameString, (char *)"str_col2", strlen("str_col2"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to change ion_string_assign_cstr take a const char *? Wouldn't that be preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be preferred, but I didn't want to make a potentially API breaking change. I need to dig deeper into what, if any, impact changing a const would have on a C API.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Maybe just put a comment on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, looking more into it, the API change wouldn't be too problematic. The ABI would remain the same, and since the const would determine our usage of the argument, it shouldn't cause an issue with users. However, I misremembered what ion_string_assign_cstr did, and it does not copy the string. So, it would have to drop the const anyway inside the function in order to pass it to the ION_STRING.

So now my question is: what is the intended use of ION_STRING? Looking at the API, most of the functions look like the string is expected to be immutable. If that is the case then ION_STRING's value field could be changed to a const BYTE *value (pointer to constant BYTE) so that the immutable nature of the string is communicated, and then the const-correctness could bubble up to the other functions.

That will probably require a lot more investigation into how the API is used.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea. I briefly pondered why ION_STRING couldn't be const. Honestly I don't know.

ION_ASSERT_OK(ion_writer_write_field_name(writer, &fieldNameString));

ion_string_assign_cstr(&value_string, "str_val1", strlen("str_val1"));
ion_string_assign_cstr(&value_string, (char *)"str_val1", strlen("str_val1"));
ION_ASSERT_OK(ion_writer_write_string(writer, &value_string));

ion_string_assign_cstr(&fieldNameString, "str_col3", strlen("str_col3"));
ion_string_assign_cstr(&fieldNameString, (char *)"str_col3", strlen("str_col3"));
ION_ASSERT_OK(ion_writer_write_field_name(writer, &fieldNameString));

ion_string_assign_cstr(&value_string, "str_val1", strlen("str_val1"));
ion_string_assign_cstr(&value_string, (char *)"str_val1", strlen("str_val1"));
ION_ASSERT_OK(ion_writer_write_string(writer, &value_string));

ION_ASSERT_OK(ion_writer_finish_container(writer));
Expand Down
4 changes: 2 additions & 2 deletions tools/cli/argtable/argtable3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ static void merge(void* data, int esize, int i, int j, int k, arg_comparefn* com
mpos = 0;

/* Allocate storage for the merged elements. */
m = (char*)xmalloc(esize * ((k - i) + 1));
m = (char*)xmalloc((size_t)esize * (size_t)((k - i) + 1));

/* Continue while either division has elements to merge. */
while (ipos <= j || jpos <= k) {
Expand Down Expand Up @@ -422,7 +422,7 @@ static void merge(void* data, int esize, int i, int j, int k, arg_comparefn* com
}

/* Prepare to pass back the merged data. */
memcpy(&a[i * esize], m, esize * ((k - i) + 1));
memcpy(&a[i * esize], m, (size_t)esize * (size_t)((k - i) + 1));
xfree(m);
}

Expand Down
2 changes: 1 addition & 1 deletion tools/ion-bench/src/ion/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace ion {
};

inline void print_iondata(const IonData &data) {
printf("[IONDATA] tpe:0x%.4X field_name:%s",
printf("[IONDATA] tpe:0x%.4lX field_name:%s",
data.tpe,
data.field_name.value_or("none").c_str()
);
Expand Down
2 changes: 1 addition & 1 deletion tools/ion-bench/src/ion/writing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ iERR write(BufferWriter<F> &writer, const IonData &val) {
WRITE_ION_TYPE(writer, tid_SYMBOL_INT, Symbol, val);
WRITE_ION_TYPE(writer, tid_TIMESTAMP_INT, std::shared_ptr<ION_TIMESTAMP>, val);
default:
printf("Attempt to write unknown type: %d\n", val.tpe);
printf("Attempt to write unknown type: %ld\n", val.tpe);
break;
}
return err;
Expand Down
2 changes: 1 addition & 1 deletion tools/ionizer/ionizer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ int main(int argc, char **argv)

fail:// this is iRETURN expanded so I can set a break point on it
if (g_ionizer_debug) {
fprintf(stderr, "\nionizer finished, returning err [%d] = \"%s\", %d\n", err, ion_error_to_str(err), (intptr_t)t);
fprintf(stderr, "\nionizer finished, returning err [%d] = \"%s\", %ld\n", err, ion_error_to_str(err), (intptr_t)t);
}

if (g_ion_debug_timer) {
Expand Down
2 changes: 1 addition & 1 deletion tools/ionsymbols/ionsymbols.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ int main(int argc, char **argv)

fail:// this is iRETURN expanded so I can set a break point on it
if (g_debug) {
fprintf(stderr, "\nionsymbols finished, returning err [%d] = \"%s\", %d\n", err, ion_error_to_str(err), (intptr_t)t);
fprintf(stderr, "\nionsymbols finished, returning err [%d] = \"%s\", %ld\n", err, ion_error_to_str(err), (intptr_t)t);
}

if (g_timer) {
Expand Down
Loading