Avoid assigning FieldPath object to *this in constructor

XMLWordPrintableJSON

    • Type: Improvement
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • Query Optimization
    • None
    • None
    • None
    • None
    • None
    • None
    • None

      SERVER-113389 created a factory function (fieldPathWithValidationStatus) to create a FieldPath and return its validation status (rather than uasserting on validation failure directly, which is what the constructor does). This introduced a lot of copied code between the factory function and the constructor, so SERVER-116314 refactored the constructor to call the factory function and use its return values. 

      Currently, this is done by assigning the FieldPath returned by the factory function to *this. However, this isn't ideal because it means that a FieldPath object and its member variables will be default initialized and then clobbered by the re-assignment. We have two options to avoid this:

      1. Refactor the factory function to take in references to the member vectors of FieldPath to be filled by the factory function. The main constructor can pass in class's member variables as references, and external callers can construct a FieldPath object using this constructor (which will need to be made public to enable calls to the factory function from outside this class).

      2. Make the FieldPath constructor FieldPath::FieldPath(std::string inputPath, bool precomputeHashes, bool validateFieldNames) private. Create a new static factory function FieldPath FieldPath::create(std::string inputPath, bool precomputeHashes, bool validateFieldNames) with a function body as below. Call the new factory method from all places that previously used the FieldPath::FieldPath(std::string inputPath, bool precomputeHashes, bool validateFieldNames) constructor.

      FieldPath FieldPath::create(std::string inputPath, bool precomputeHashes, bool validateFieldNames) {
          auto statusWithFieldPath =
              fieldPathWithValidationStatus(std::move(inputPath), precomputeHashes, validateFieldNames);
          return uassertStatusOK(std::move(statusWithFieldPath));
      }
      

      A sys-perf patch should be run on this change to ensure that it does not cause the constructor's performance to regress.

            Assignee:
            Unassigned
            Reporter:
            Natalie Hill
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: