Add phlex::identifier

[beojan]

This splits out the identifier implementation from #267, and removes the inheritance from (and implicit conversions to / from) `string_view.

There is still an explicit constructor from string_view and an explicit conversion operator to string_view.

Removing the implicit conversion necessitated implementing tag_invoke since identifier no longer satisfies is_string_like<identifier> = std::is_convertible<identifier, std::string_view>.

[beojan]

@phlexbot format

[github-actions]

No automatic markdownlint fixes were necessary.

[github-actions]

No automatic cmake-format fixes were necessary.

[github-actions]

Automatic clang-format fixes pushed (commit 5236ed9).
⚠️ Note: Some issues may require manual review and fixing.

[marcpaterno]

I am unclear about the reason for the shared state in the implementation of identifier. Are identifiers modifiable, such that it is important that modification of one identifier should be seen by copies of that identifier? I think this is not the case. Is this sharing intended only to be an optimization (perhaps so that copying identifers is cheap)? If so, I think this is a premature optimization. It may be that the state of the identifier will end up being cheap to copy. It may not be cheaper to copy the std::shared_ptr (which also involves adjusting reference counts) than the necessary state.

I am also unclear about the use of hashes. It appears this is intended to be an optimization to make comparison for equality inexpensive. I think this is a premature optimization, because we do not yet know exactly what the state should be in place, and what comparisons are necessary.

[knoepfel]

@beojan, thanks for the PR. I've added a few comments below.

[beojan]

I am unclear about the reason for the shared state in the implementation of identifier. Are identifiers modifiable, such that it is important that modification of one identifier should be seen by copies of that identifier? I think this is not the case. Is this sharing intended only to be an optimization (perhaps so that copying identifers is cheap)? If so, I think this is a premature optimization. It may be that the state of the identifier will end up being cheap to copy. It may not be cheaper to copy the std::shared_ptr (which also involves adjusting reference counts) than the necessary state.

The identifiers are (semi) immutable (i.e. you can't mutate the identifier, but you can replace it wholesale with the assignment operator). The shared_ptr is indeed to make it cheaper to copy the identifiers. I can replace it with just the string.

I am also unclear about the use of hashes. It appears this is intended to be an optimization to make comparison for equality inexpensive. I think this is a premature optimization, because we do not yet know exactly what the state should be in place, and what comparisons are necessary.

Here, I disagree that this is a premature optimization. The way identifiers are used, they're looked up in sets and maps a lot. This requires fast equality comparison and fast less-than comparison. They need to have a fixed ordering but crucially, this doesn't need to be alphabetical. By using hashes for this ordering we get the fast lookups we need.

We're already doing something like this manually in data_cell_index (https://github.com/Framework-R-D/phlex/blob/main/phlex/model/data_cell_index.cpp) by carrying hashes around along with names, and using them for lookups.

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
phlex/model/identifier.cpp	82.35%	2 Missing and 1 partial ⚠️
phlex/model/identifier.hpp	85.71%	1 Missing ⚠️

❌ Your project status has failed because the head coverage (75.32%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   74.28%   75.32%   +1.04%     
==========================================
  Files         124      126       +2     
  Lines        2955     2954       -1     
  Branches      513      519       +6     
==========================================
+ Hits         2195     2225      +30     
+ Misses        540      505      -35     
- Partials      220      224       +4     

Flag	Coverage Δ
unittests	75.32% <84.61%> (+1.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
phlex/configuration.cpp	87.50% <100.00%> (+64.09%)	⬆️
phlex/configuration.hpp	95.23% <ø> (ø)
phlex/model/identifier.hpp	85.71% <85.71%> (ø)
phlex/model/identifier.cpp	82.35% <82.35%> (ø)

... and 7 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20c5955...1d1e4ad. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greenc-FNAL]

Review the full CodeQL report for details.

[beojan]

@phlexbot format

[github-actions]

No automatic markdownlint fixes were necessary.

[github-actions]

No automatic cmake-format fixes were necessary.

[github-actions]

No automatic jsonnetfmt fixes were necessary.

[github-actions]

Automatic clang-format fixes pushed (commit 110f528).
⚠️ Note: Some issues may require manual review and fixing.