search

RosettaCommons / rosetta

The Rosetta Bio-macromolecule modeling package.
https://www.rosettacommons.org
Other
116 stars 55 forks source link

clang 16 (rightly, imho) points to test on variable address to be != NULL, which is always true - stops compiling #30

Closed smoe closed 5 months ago

smoe commented 6 months ago

Clang kindly pointed to (and then stopped compilation) this reference that is compared against the null pointer, which indeed looks suspicious:

external/cifparse/TableFile.C:694:19: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to false [-Wtautological-undefined-compare]
            if (&(_blocks[blockI]._tables[tableI]) == nullptr)
                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    ~~~~~~~
external/cifparse/mapped_ptr_vector.h:205:8: note: 'operator[]' returns a reference
    T& operator[](unsigned int index);
       ^
external/cifparse/TableFile.C:759:19: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to false [-Wtautological-undefined-compare]
            if (&(_blocks[blockI]._tables[tableI]) == nullptr)
                 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    ~~~~~~~
external/cifparse/mapped_ptr_vector.h:205:8: note: 'operator[]' returns a reference
    T& operator[](unsigned int index);
       ^
external/cifparse/TableFile.C:916:11: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to true [-Wtautological-undefined-compare]
    if (&(_blocks[blockIndex]._tables[tableIndex]) != nullptr)
         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    ~~~~~~~
external/cifparse/mapped_ptr_vector.h:205:8: note: 'operator[]' returns a reference
    T& operator[](unsigned int index);

My immediate instinct was to just remove the "&" to check if _tables is NULL at position tableIndex, but besides that &(.,,,)-construct being used throughout that file, this runs into a null-arithmetic error since that table does not hold pointers.

roccomoretti commented 5 months ago

cifparse is technically external code (but "vendorized" into the repo). We generally take a "light touch" with external code, only fixing what's absolutely necessary. We're not warnings clean with external code, but we shouldn't error out for the warnings (we do enable -Werror for Rosetta code, but that should be off for external code).

I'm seeing the same warnings reported for Clang 13, but on my machine it's not erroring out for those issues. From my perspective, the issue would be that compilation stops, no that the warning messages get printed.

roccomoretti commented 5 months ago

If you think this message is still giving issues, can you post the full compile log? (Subsequent invocations of scons should print less before erroring out. If you run twice with -j1, the second run should have just the command that errored out.)

smoe commented 5 months ago

Seems like I indeed erred wrt the cause of the stop. My compilation of the main Rosetta source tree has succeeded in the mean time. Sorry for the noise.