Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-280

Remove or disable daf::base::Citizen

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None

      Description

      Citizen class is a C++ base class that is designed to help debug memory leaks. Its SDSS predecessor (C, not C++) was apparently incredibly useful, but with our use of smart pointers and RAII, I believe it's essentially been a non-issue. I can't recall a single instance in which it was helpful in debugging a memory problem, and it's the source of a constant trickle of confusion due to failures to call Citizen::markPersistent and the inherited getId method looking like it returns some other kind of ID.

      I think we should at least #define out the Citizen implementation by default. That'll make it truly a zero-cost utility in non-debug builds, and let us use it in more places (right now, we don't use it for small classes, which decreases its utility even further). If we do that, I'd also advocate changing the interface to only provide access via a separate class (see DM-675).

      But I think we could save the most effort by just removing it entirely, as that'll save on future maintenance effort (such as modifying the threading stuff to use C++11 constructs, or just making sure it works properly with pybind11, which is pickier than Swig about returning raw pointers). I'd like to specifically ping Robert Lupton on that question (as well as the broader usual audience for RFCs) - I know he agrees that Citizen isn't as useful as it was in C, but I'm very interested in his thoughts about removing it entirely.

      I'd also be interested to hear from people coming from other C++ projects as to whether they used (or wish they'd had) similar classes.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch this RFC has bubbled up again.

            Show
            tjenness Tim Jenness added a comment - Jim Bosch this RFC has bubbled up again.
            Hide
            tjenness Tim Jenness added a comment -

            I've moved the planned end to DMLT week.

            Show
            tjenness Tim Jenness added a comment - I've moved the planned end to DMLT week.
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch this RFC is going to go past it's planned end date again today.

            Show
            tjenness Tim Jenness added a comment - Jim Bosch this RFC is going to go past it's planned end date again today.
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch are you ready to withdraw this RFC yet?

            Show
            tjenness Tim Jenness added a comment - Jim Bosch are you ready to withdraw this RFC yet?
            Hide
            jbosch Jim Bosch added a comment -

            After a discussion with Robert Lupton, we've agreed to adopt this with the more expansive version of the original proposal: Citizen and all use of it can be completely removed.

            We also agreed one major role filled by Citizen's SDSS predecessor - tracking the provenance of individual astronomical objects through merge and split operations - ought to be met by code tied more directly to afw::table, but that designing that system will take some thought and is not an immediate priority.

            Show
            jbosch Jim Bosch added a comment - After a discussion with Robert Lupton , we've agreed to adopt this with the more expansive version of the original proposal: Citizen and all use of it can be completely removed. We also agreed one major role filled by Citizen's SDSS predecessor - tracking the provenance of individual astronomical objects through merge and split operations - ought to be met by code tied more directly to afw::table, but that designing that system will take some thought and is not an immediate priority.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Watchers:
              Gregory Dubois-Felsmann, Jim Bosch, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Paul Price, Pim Schellart [X] (Inactive), Robert Lupton, Russell Owen, Tim Jenness
              Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  Jenkins

                  No builds found.