- changed status to open
- removed comment
Carpet may call object methods with this == NULL
see pull request https://bitbucket.org/eschnett/carpet/pull-requests/7/carpet-avoid-using-null-pointer-for-member/diff
Keyword:
Comments (12)
-
reporter -
reporter - changed status to resolved
- removed comment
This is likely not worthwhile to address (if indeed it is a bug) unless we encounter a system where this breaks (with a SEGFAULT so it won't be a silent failure). See https://bitbucket.org/eschnett/carpet/pull-requests/7/carpet-avoid-using-null-pointer-for-member/diff#comment-10904860
-
reporter - changed status to open
- removed comment
Fails in CarpetPeriodic test testperiodicinterp. Related to
#1821which is however only a warning. -
- removed comment
I now recall the correction I put into place. The fix is applied at the calling side (obviously). Instead of creating a temporary object, we're using another object that already exists at the caller, e.g.
src
instead ofdst
.I thought this fix was already committed. Maybe I only applied it to a branch, or maybe I missed a calling site?
-
reporter - removed comment
maybe, here's another way about it: https://bitbucket.org/eschnett/carpet/pull-requests/7/carpet-avoid-using-null-pointer-for-member/diff
-
- removed comment
(1) I don't think that CarpetLib itself calls this method, so the changes to
ggf.cc
should not be necessary. (2) InPeriodicCarpet
, you could usesrc
instead offake_data_pointer()
. (3) The initialization of the static variable infake_data_pointer
is not thread-safe. This has actually become an issue when using Qthreads for parallelization. -
reporter - removed comment
(1) I don't think that CarpetLib itself calls this method, so the changes to
ggf.cc
should not be necessary.I added a comment to the code diff: https://bitbucket.org/eschnett/carpet/pull-requests/7/carpet-avoid-using-null-pointer-for-member/diff#comment-33242870
(2) In
PeriodicCarpet
, you could usesrc
instead offake_data_pointer()
. Could be done. That seems odd though.(3) The initialization of the static variable in
fake_data_pointer
is not thread-safe. This has actually become an issue when using Qthreads for parallelization. Correct. At the time I reported this CarpetLib was not yet thread safe (ie none of the Timers was) so it did not matter. Thread safe would be either using new() or the src trick that you suggest or making transfer_from a static member function that takes the current dst as eg its first argument. -
reporter - changed status to open
- removed comment
I updated the pull request with a version that introduces static functions for gdata::copy_from and gdata::transfer_from and renames them to copy_data and transfer_data. This requires changes in all code using those functions since the originals are gone (could be restored but the current state brings attention to possible code parts that may also pass in a NULL this pointer).
-
reporter - removed comment
Erik: I think this should be reviewed before the next release as otherwise CarpetPeriodic fails to run when compiled with newer compilers (gcc 6.0 at least).
-
reporter - changed milestone to ET_2017_05
- removed comment
-
- changed status to open
- removed comment
-
reporter - changed status to resolved
- removed comment
Thank you. Applied as git hash 474a09de0c1cd56c00492400fe5304c8e9591cbd "PeriodicCarpet: avoid using NULL this pointer for gdata::copy_from" of Carpet.
- Log in to comment