Fix bad uses of sizeof(char[])/sizeof(char*)

[PhilMiller]

In both cases, we're ultimately getting the length 6 of the literal string "{{id}}", without '\0' terminator

In the case where the literal string is passed to sizeof, it's obtusely correct, because that evaluates the literal char[], which takes serious language-lawyering to recognize. It's also very fragile to array-to-pointer-decay if someone attempts to refactor the code in some way, which would leave it over by one.

In the case where std::string pattern is a function parameter and the code says sizeof(pattern.c_string()) - 2, it's only managed to operate correctly because all of the current call sites pass an argument of "{{id}}", and it's only tested on 64-bit systems.

Changes

• Replace two instances of sizeof(<string literal>) with std::string::size() to be more durably correct

Todos

• @robertbartel offered to add tests for this as appropriate

Checklist

• PR has an informative and human-readable title
• Changes are limited to a single goal (no scope creep)
• Code can be automatically merged (no conflicts)
• Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

[PhilMiller]

Tagged @robertbartel and @christophertubbs as reviewers, as they introduced the offending code.

[christophertubbs]

Good find.

I have a few thoughts about it (criticisms of my original code, really) that I want to run by you that might improve it more.

1. Should patterns like filepattern = filepattern.replace(id_index, id_pattern.size(), identifier); be replaced with just filepattern.replace(id_index, id_pattern.size(), identifier); since the reassignment is redundant?
2. Should lines like int id_index = filepattern.find(id_pattern); be replaced with std::string::size_type id_index = filepattern.find(id_pattern); in order to avoid any sort of casting issues?
3. Is there a clean way to move lines like std::string id_pattern = "{{id}}"; into higher level constants in order to sort of rid it of a its magic-stringiness and possibly centralize its definition so it may be used in other contexts (don't know where)? Its value has already changed once ("{{ID}}" => "{{id}}") and appears to be a sort of keyword within the config. If it's some sort of standard, it may pay to move it outside of that function. It looks like that string is also used in some of the BMI code. If it changes in one place I imagine it should change in the other.

I'm obviously not the expert on this, so I assume you'll know the nuts and bolts of it better.

[robertbartel]

Good find @PhilMiller.

I tend to think "yes" regarding @christophertubbs's questions 1 and 2 above. Regarding question 3, eh, maybe. I do think we should (more fully) abstract this logic out to a single reusable utility function somewhere. I feel like I was probably starting to do this by adding the CatchmentFormulation.config_pattern_substitution function, but didn't really position it so it could be plugged in everywhere it was needed (and clearly I wasn't thinking hard enough about things when I added it). If/after we do that, it may be more apparent whether and where to define strings constants like {{id}}.

Also, even if the above isn't done, clearly some additional tests are needed, at minimum to ensure CatchmentFormulation.config_pattern_substitution behaves robustly enough. I will work on contributing those.

[PhilMiller]

I've addressed Chris's points 1 and 2. I think 3 is out of scope of this change, whose goal is to replace code that happens to work, but isn't really 'correct'.

[PhilMiller]

This'll be rebased on #955, after finding that the impacted functionality was not covered by tests, and was in fact broken.