Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9249

Modify FlagHandler C++ and flagDecorator.py to make flag identification robust

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None
    • Story Points:
      8
    • Sprint:
      DRP S17-3, DRP S17-4
    • Team:
      Data Release Production

      Description

      As discovered by DM-6561, the FlagHandler mechanism for connecting the enumeration of flags in C++ and the order in which the flags appear in the schema and internal FlagHandler structures is not robust. Fix this, problem, so that the identifier used identify a particular flag and the lookup of the Flag Key are guaranteed to match.

      Then fix the flagDecorator (it will be simpler) and all of the algorithms to match the new FlagHandler scheme.

        Attachments

          Issue Links

            Activity

            Hide
            rearmstr Bob Armstrong added a comment -

            I looked through all the code, but mostly focused on meas_base. It looks good. There are only a few minor comments that need to be addressed on the pull request.

            Show
            rearmstr Bob Armstrong added a comment - I looked through all the code, but mostly focused on meas_base. It looks good. There are only a few minor comments that need to be addressed on the pull request.
            Hide
            pgee Perry Gee added a comment -

            Bob Armstrong

            About your suggestion that the add methods could return FlagDefinition const &, I didn't think that this would be safe with the data stored in an underlying std::vector<FlagDefinition>. At one point, I was writing my own storage class to fix this problem, but Jim asked me not to. Can you think of a way to make this safe? I definitely saw problems with the std::vector when I used this class from Python.

            Show
            pgee Perry Gee added a comment - Bob Armstrong About your suggestion that the add methods could return FlagDefinition const &, I didn't think that this would be safe with the data stored in an underlying std::vector<FlagDefinition>. At one point, I was writing my own storage class to fix this problem, but Jim asked me not to. Can you think of a way to make this safe? I definitely saw problems with the std::vector when I used this class from Python.
            Hide
            rearmstr Bob Armstrong added a comment -

            I'm don't think was my suggestion. I was actually talking about the arguments to the function.

            But as to your question, I think you were having problems because you are adding elements to the vector via push_back which would invalidate the references? Not because of the underlying storage?

            Show
            rearmstr Bob Armstrong added a comment - I'm don't think was my suggestion. I was actually talking about the arguments to the function. But as to your question, I think you were having problems because you are adding elements to the vector via push_back which would invalidate the references? Not because of the underlying storage?
            Hide
            pgee Perry Gee added a comment -

            Bob Armstrong Yes, I was adding elements. I wasn't expecting it to be stable. I was just saying that there would be an advantage to using a storage class with underlying stable memory allocation, as pointers to array members would then be stable. It was a small thing. Making a copy seems reasonable in this case.

            Show
            pgee Perry Gee added a comment - Bob Armstrong Yes, I was adding elements. I wasn't expecting it to be stable. I was just saying that there would be an advantage to using a storage class with underlying stable memory allocation, as pointers to array members would then be stable. It was a small thing. Making a copy seems reasonable in this case.
            Hide
            jbosch Jim Bosch added a comment -

            Was merged to master on Friday.

            Show
            jbosch Jim Bosch added a comment - Was merged to master on Friday.

              People

              • Assignee:
                pgee Perry Gee
                Reporter:
                pgee Perry Gee
                Reviewers:
                Bob Armstrong
                Watchers:
                Bob Armstrong, Jim Bosch, John Swinbank, Perry Gee
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: