- changed status to open
- removed comment
this compared to NULL in CarpetLib
I get
/home/rhaas/Cactus/arrangements/Carpet/CarpetLib/src/gdata.cc: In member function ‘void gdata::copy_from(comm_state&, const gdata*, const ibbox&, const ibbox&, const islab*, int, int)’:
/home/rhaas/Cactus/arrangements/Carpet/CarpetLib/src/gdata.cc:185:53: warning: nonnull argument ‘this’ compared to NULL [-Wnonnull-compare]
int const order_space = (this ? cent : src->cent) == vertex_centered ? 1 : 0;
~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
which leads me to suspect that the compiler is allowed to assume this != NULL and will optimize out one half of the condition.
Keyword:
Comments (8)
-
-
reporter - removed comment
That will take care of the NULL this I guess as long as srcs is never empty (in which case I'd expect AT to assert() or if not we'll have undefined behaviour since the pointer src [while presumably not "really" used], is not a pointer to a gdata object).
I am not sure how much can be reviewed in this. If this patch works for the test cases then "Please apply". Should we include the patch in the test suites on at least some machines?
-
- changed status to open
- removed comment
-
reporter - changed status to open
- removed comment
This fails all the time for me. The first line:
int const order_space = cent == vertex_centered ? 1 : 0;
will fail when this is NULL and this happens to me of the periodiccarpet test testperiodicinterp using 2 MPI ranks.
-
- removed comment
Wasn't there a proposed solution that replaces
cent
bythis ? cent : src->cent
? -
reporter - removed comment
No,
this ? cent : src->cent
is what the current (in master) code does and that gives the "this compared to NULL warning". The underlying issue is using a NULL this pointer to call member functions. In the case of PeriodicCarpet this happens when doing dst->copy_from(...) with a NULL dst when dst is not a local component.
I had a fix for that but it involved calling new_typed_data() when dst would otherwise be NULL. Initially the thought was that new_typed_data() may be too expensive. I am no longer so sure since transfer_from itself already creates a new gdata for its buf object all the time. I am however now working on a (not thread safe...) replacement that uses a single typed_data for all such occurrences.
-
reporter - removed comment
I re-opened
#1821which had the original proposed fix. I'll put the updated fix with only a single new_typed_data into the old pull request. -
reporter - changed status to resolved
- removed comment
Fixed along with
#1821in git hash 474a09de0c1cd56c00492400fe5304c8e9591cbd "PeriodicCarpet: avoid using NULL this pointer for gdata::copy_from" of Carpet. - Log in to comment
I have a two-line correction for this that I am currently testing: