this compared to NULL in CarpetLib

Comments (9)

1. 

   Erik Schnetter

   • changed status to open
   • removed comment

   I have a two-line correction for this that I am currently testing:

   diff --git a/CarpetLib/src/gdata.cc b/CarpetLib/src/gdata.cc
   index 3d448fd..1ff8cc6 100644
   --- a/CarpetLib/src/gdata.cc
   +++ b/CarpetLib/src/gdata.cc
   @@ -183,7 +183,7 @@ void gdata::copy_from(comm_state &state, gdata const *const src,
      vector<gdata const *> const srcs(1, src);
      CCTK_REAL const time = 0.0;
      vector<CCTK_REAL> const times(1, time);
   -  int const order_space = (this ? cent : src->cent) == vertex_centered ? 1 : 0;
   +  int const order_space = cent == vertex_centered ? 1 : 0;
      int const order_time = 0;
      transfer_from(state, srcs, times, dstbox, srcbox, slabinfo, dstproc, srcproc,
                    time, order_space, order_time);
   @@ -221,7 +221,7 @@ void gdata::transfer_from(comm_state &state, vector<gdata const *> const &srcs,
          assert(srcs.AT(t)->has_storage());
        }
      }
   -  gdata const *const src = is_src ? srcs.AT(0) : NULL;
   +  gdata const *const src = srcs.AT(0);
   
      operator_type const my_transport_operator =
          is_dst ? transport_operator : src->transport_operator;
   

   • 2016-11-23T12:34:19+00:00
2. 

   Roland Haas 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?

   • 2016-11-23T12:42:22+00:00
3. 

   Erik Schnetter

   • changed status to open
   • removed comment

   • 2017-02-08T10:07:54+00:00
4. 

   Roland Haas 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.

   • 2017-03-02T15:37:10+00:00
5. 

   Erik Schnetter

   • removed comment

   Wasn't there a proposed solution that replaces cent by this ? cent : src->cent?

   • 2017-03-02T15:41:38+00:00
6. 

   Roland Haas 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.

   • 2017-03-02T16:09:05+00:00
7. 

   Roland Haas reporter

   • removed comment

   I re-opened #1821 which had the original proposed fix. I'll put the updated fix with only a single new_typed_data into the old pull request.

   • 2017-03-02T16:53:11+00:00
8. 

   Roland Haas reporter

   • changed status to resolved
   • removed comment

   Fixed along with #1821 in git hash 474a09de0c1cd56c00492400fe5304c8e9591cbd "PeriodicCarpet: avoid using NULL this pointer for gdata::copy_from" of Carpet.

   • 2017-06-13T13:52:18+00:00
9. 

   Roland Haas reporter

   • edited description
   • changed status to closed

   • 2019-02-21T19:59:43+00:00
10. Log in to comment