search

amazon-ion / ion-python

A Python implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
261 stars 51 forks source link

C Extension warning cleanup #349

Closed nirosys closed 3 months ago

nirosys commented 7 months ago

Issue #, if available: n/a

Description of changes: Prior to this PR compilation of the C extension resulted in quite a bit of noise from compiler warnings and associated notes:

$ clang -fno-strict-overflow -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -Iamazon/ion/ion-c-build/include -Iamazon/ion/ion-c-build/include/ionc -Iamazon/ion/ion-c-build/include/decNumber -I/usr/local/opt/python@3.12/Frameworks/Python.framework/Versions/3.12/include/python3.12 -c amazon/ion/ioncmodule.c -o build/temp.macosx-13.0-x86_64-cpython-312/amazon/ion/ioncmodule.o -std=c99 2>&1 | wc -l
      82

The warnings consisted of the following:

amazon/ion/ioncmodule.c:183:10: warning: assigning to 'char *' from 'const char *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
amazon/ion/ioncmodule.c:184:5: warning: unused label 'fail' [-Wunused-label]
amazon/ion/ioncmodule.c:546:54: warning: cast to 'ION_TYPE' (aka 'struct ion_type *') from smaller integer type 'int' [-Wint-to-pointer-cast]
amazon/ion/ioncmodule.c:715:53: warning: cast to 'ION_TYPE' (aka 'struct ion_type *') from smaller integer type 'int' [-Wint-to-pointer-cast]
amazon/ion/ioncmodule.c:740:57: warning: cast to 'ION_TYPE' (aka 'struct ion_type *') from smaller integer type 'int' [-Wint-to-pointer-cast]
amazon/ion/ioncmodule.c:767:5: warning: unused label 'fail' [-Wunused-label]
amazon/ion/ioncmodule.c:798:21: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
amazon/ion/ioncmodule.c:812:14: warning: unused variable 'last_element' [-Wunused-variable]
amazon/ion/ioncmodule.c:786:5: warning: variable 'writer' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
amazon/ion/ioncmodule.c:779:9: warning: variable 'writer' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
amazon/ion/ioncmodule.c:1109:28: warning: comparison between pointer and integer ('ION_TYPE' (aka 'struct ion_type *') and 'int') [-Wpointer-integer-compare]
amazon/ion/ioncmodule.c:1516:12: warning: incompatible pointer types returning 'ionc_read_Iterator *' from a function with result type 'PyObject *' (aka 'struct _object *') [-Wincompatible-pointer-types]

Most of these aren't too concerning, just noisy. However, the warnings about potential use of the uninitialized writer could result in a segfault under error conditions. If the uninitialized writer was non-zero, ion_writer_close would be called, which could result in dereference of the invalid pointer.

It would probably be worthwhile to upgrade warnings into errors for the c extension itself.

After this PR:

$ clang -fno-strict-overflow -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -Iamazon/ion/ion-c-build/include -Iamazon/ion/ion-c-build/include/ionc -Iamazon/ion/ion-c-build/include/decNumber -I/usr/local/opt/python@3.12/Frameworks/Python.framework/Versions/3.12/include/python3.12 -c amazon/ion/ioncmodule.c -o build/temp.macosx-13.0-x86_64-cpython-312/amazon/ion/ioncmodule.o -std=c99
$

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rmarrowstone commented 7 months ago

Can't say my C-chops are any better than Matt's, but the changes make sense to me. Thanks!!!