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

afw::table::SchemaMapper::addMapping calls wrong overload with string literal

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      0.1
    • Epic Link:
    • Sprint:
      DRP S18-4, DRP S18-5, DRP S18-6
    • Team:
      Data Release Production

      Description

      Running clang-tidy readability-implicit-bool-cast on src/detection/Footprint.cc revealed that the wrong overload of SchemaMapper::addMapping is called when given a string literal as the second argument.

      We expect it to call Key<T> addMapping(Key<T> const& inputKey, std::string const& outputName, bool doReplace = true);, but instead it calls Key<T> addMapping(Key<T> const& inputKey, bool doReplace = false); because it prefers to cast to bool rather than create a std::string temporary.

      I propose we add an overload taking char const*, make it primary and redirect the std::string const& overload to it.
      Another alternative would be to find where it is called with a string literal and change it to a std::string.

        Attachments

          Activity

          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Which I guess also limits the scope of the problem.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Which I guess also limits the scope of the problem.
          Hide
          krzys Krzysztof Findeisen added a comment -

          Pim Schellart [X] could we maybe get an outsider to review this? I think the final implementation (forbid calling addMapping with C strings) might seem weird to somebody who wasn't following the above discussion, and it's best to address any such usability issues now.

          Show
          krzys Krzysztof Findeisen added a comment - Pim Schellart [X] could we maybe get an outsider to review this? I think the final implementation (forbid calling addMapping with C strings) might seem weird to somebody who wasn't following the above discussion, and it's best to address any such usability issues now.
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Sounds good. I'll ask someone else.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Sounds good. I'll ask someone else.
          Hide
          rowen Russell Owen added a comment -

          Looks good to me. Sorry that I forgot to mark this as reviewed here (after doing so on github nearly 2 weeks ago).

          Show
          rowen Russell Owen added a comment - Looks good to me. Sorry that I forgot to mark this as reviewed here (after doing so on github nearly 2 weeks ago).
          Hide
          pschella Pim Schellart [X] (Inactive) added a comment -

          Merged after a bit of a delay.

          Show
          pschella Pim Schellart [X] (Inactive) added a comment - Merged after a bit of a delay.

            People

            Assignee:
            pschella Pim Schellart [X] (Inactive)
            Reporter:
            pschella Pim Schellart [X] (Inactive)
            Reviewers:
            Russell Owen
            Watchers:
            Jim Bosch, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.