Late review/suggestions for #136

[greenc-FNAL]

I was still trying to produce a readable composite review (Copilot/me) of #136 when it was approved and merged.

Below, please find Copilot's analysis of #136 followed by two patches it generated to address some of its findings:

---

Thanks — I analyzed the full diff you attached (git diff -w --minimal upstream/main). Below I list the terminology/naming changes I found, grouped by concept, and then I call out anomalies >(misspellings, inconsistent terminology, likely-missed edits, and a few places that could cause build/semantic surprises). At the end I note recommended fixes and next steps I can take if you want.

What I did

• Read the unified diff you provided and walked each changed file to extract every rename, file rename, and API change.
• Verified cross-file consistency where practical (includes, typedefs, function signatures, class names, tests).
• Looked for leftover uses of the old terminology, spelling/guard mistakes, and naming inconsistencies.

Summary (global mapping of old → new)

• level_id → data_cell_index

  • level_id.hpp / level_id.cpp deleted
  • data_cell_index.hpp / data_cell_index.cpp added
  • level_id::hash_type → data_cell_index::hash_type
  • level_id_ptr → data_cell_index_ptr
  • level_id::base_ptr() → data_cell_index::base_ptr()
  • level_id::level_name() → data_cell_index::layer_name()
  • level_id::level_hash() → data_cell_index::layer_hash()
  • methods: make_child(...) use new_layer_name argument name

• level_counter → data_cell_counter

  • level_counter.hpp / .cpp deleted
  • data_cell_counter.hpp / .cpp added
  • flush_counts uses data_cell_index::hash_type (was level_id::hash_type)

• level_hierarchy → data_layer_hierarchy

  • level_hierarchy.hpp / .cpp renamed → data_layer_hierarchy.hpp / .cpp
  • level_entry → layer_entry
  • levels_ map → layers_ map (inside hierarchy files)

• "level" (as a concept) → "layer" (as user-facing term in many places)

  • e.g. product_query.family → product_query.layer
  • product_query.operator()(std::string family) → operator()(std::string layer)
  • product_matcher.level_path → product_matcher.layer_path
  • product_matcher-level_path encode / error messages changed to mention “layer”
  • Many comments changed from “level(s)” to “layer(s) / data layer”

• level-related APIs in product_store and core

  • product_store::level_name() → product_store::layer_name()
  • product_store::parent(level_name) → parent(layer_name)
  • product_store constructors now use new_layer_name parameter names
  • product_store::id() now returns data_cell_index_ptr (instead of level_id_ptr)

• Naming in concurrency/graph primitives

  • level_sentry → layer_sentry (class)
  • member types updated accordingly (std::stack<layer_sentry>), but see anomaly re: member variable name below

• Many tests and examples updated to use the new types/terms:

  • test filenames: level_counting.cpp → data_cell_counting.cpp, level_id.cpp → data_cell_index.cpp
  • many test helpers, test expectations, and strings updated.

• CMake

  • model/CMakeLists.txt: removed level_* files and added data_cell_counter.cpp, data_layer_hierarchy.cpp, data_cell_index.cpp

Files renamed or added / removed (high-level)

• Removed:

  • phlex/model/level_id.hpp (deleted)
  • phlex/model/level_id.cpp (deleted)
  • phlex/model/level_counter.hpp/.cpp (deleted)
  • phlex/model/level_hierarchy.hpp/.cpp (renamed)

• Added:

  • phlex/model/data_cell_index.hpp/.cpp
  • phlex/model/data_cell_counter.hpp/.cpp
  • phlex/model/data_layer_hierarchy.hpp/.cpp (rename of level_hierarchy.*)

• Many source files updated to include the new headers and to use the new types/names.

All concrete symbol renames (representative list)

• level_id -> data_cell_index (class)
• level_id_ptr -> data_cell_index_ptr
• level_hash -> layer_hash
• level_name -> layer_name
• level_counter -> data_cell_counter
• level_hierarchy -> data_layer_hierarchy
• level_entry -> layer_entry
• levels_ (in hierarchy) -> layers_ (map in hierarchy implementation)
• level_sentry -> layer_sentry
• product_query.family -> product_query.layer
• product_query.operator()(std::string family) -> operator()(std::string layer)
• product_matcher.level_path -> product_matcher.layer_path
• product_store::level_name() -> product_store::layer_name()
• message_sender(level_hierarchy&) -> message_sender(data_layer_hierarchy&)
• Many variables, parameters, messages, and test strings swapped "level" → "layer" or "data cell" where appropriate.

Anomalies, inconsistencies, and likely missed issues
I list items I found that look incorrect, inconsistent, or worth fixing before merging.

1. Header-guard / macro name mismatch in data_cell_index.hpp

• File: phlex/model/data_cell_index.hpp
• Problem: header guard is "#ifndef PHLEX_MODEL_DATA_CELL_ID_HPP" and the corresponding #endif comment is "PHLEX_MODEL_DATA_CELL_ID_HPP". The filename is data_cell_index.hpp — the typical guard would be PHLEX_MODEL_DATA_CELL_INDEX_HPP (INDEX vs ID). This is a bug (typo) in the guard macro name. It risks:
  • Confusion and potential collisions if another header uses the same macro.
  • Makes the guard inconsistent with the rest of repo naming conventions.
• Fix: change guard to PHLEX_MODEL_DATA_CELL_INDEX_HPP (both ifndef/define and the endif comment).

2. Inconsistent terminology: mixing "cell" and "layer"

• Observed: class names use both data_cell_index and data_layer_hierarchy. The hierarchy is named data_layer_hierarchy while the index type is data_cell_index. In many comments the project uses "data layer", and product_query uses "layer". This creates a conceptual mismatch:
  • Are they referring to "data layer" or "data cell"? The index type uses "cell", the hierarchy uses "layer".
• Impact: semantic confusion for maintainers and possible naming mismatches in future code. Either:
  • Use data_layer_index / data_layer_id (consistent layer terminology), or
  • Rename data_layer_hierarchy -> data_cell_hierarchy (consistent cell terminology),
  • Or clearly document the intended difference between “cell” and “layer” if deliberate.
• I suggest picking one term (layer or cell) and applying consistently.

3. Residual variable names using "levels_" in framework_graph

• File: phlex/core/framework_graph.hpp
• The container member was changed from std::stack<level_sentry> to std::stack<layer_sentry> (type updated), but the member name remains levels_ (it was not renamed levels_ → layers_).
• This is an inconsistent identifier (type says layer, name says levels). It’s merely cosmetic but reduces readability; suggest renaming the member to layers_ (or keep consistent choice).

4. Some comments or names still say "level"

• I found a few comments and string literals that still use "level" rather than the new wording. Examples:
  • Some small comments (e.g. // FIXME: only intended to be used in a single-threaded context as std::map is not thread-safe.) unaffected, but any user-facing message or documentation content referencing "level" should be verified for consistency.
• Not critical for compilation but good to tidy.

5. Header/include ordering / redundancies

• Several headers now include both data_cell_index.hpp and fwd.hpp (which itself forward-declares data_cell_index). This is harmless but redundant. Not a correctness bug, just minor cleanup possible.

6. product_query helper to_layer signature

• File: phlex/core/product_query.hpp
• to_layer(product_query& label) is defined but takes a non-const reference. The equivalent to_name uses a const&. This mirrors original to_family behavior, but consider whether a const overload (or returning const ref) is desired. Not strictly a bug but something to review for API ergonomics.

7. Typo / inconsistent header-guard name also appears in comment at end of data_cell_index.hpp:

• I note the same PHLEX_MODEL_DATA_CELL_ID_HPP appears there; fix both.

8. Potential public/ABI breakages (for consumers)

• These renames change public APIs and symbol names (product_store::level_name → layer_name, data types renamed). That will break external user code and require refactors or shims/aliases if backwards compatibility is desired.
• If you need backwards compatibility consider adding deprecated aliases (inline functions or typedefs) for a short transition period.

9. Changed map key types and containers — double-check concurrency container semantics

• Example: declared_fold.hpp changed results_ from a concurrent_unordered_map<level_id, unique_ptr<R>> to concurrent_unordered_map<data_cell_index, unique_ptr<R>>. That requires data_cell_index to be safely usable as a key (hash specialization and equality). I saw std::hash specialization for data_cell_index in the header; operator== implemented in .cpp. That looks OK, but it's a behavioral change (previously keys may have been pointers or hashes — check expectations).
• Similarly tbb concurrent maps that previously used hash_type keys were moved to using the type itself in at least one place — this should compile but please ensure the intended semantics remain the same.

10. Minor: message and test text consistency

• Many tests and examples were updated successfully. A few commented-out regex / commented examples in test/product_matcher.cpp still show both optional_layer_path and commented code — not harmful but could be cleaned.

Potential build / runtime concerns to double-check

• Header-guard mismatch (item Optionally build with experimental FORM integration #1) is the most immediate correctness issue to fix.
• Ensure all includes that used to forward-declare level_id still now include data_cell_index if they use members; some headers added the necessary include but scan the build to be sure no missing includes remain.
• Because multiple public names changed, any external users must be updated.

Concrete places to fix (file + quick patch description)

• phlex/model/data_cell_index.hpp
  • Change header guard macros to PHLEX_MODEL_DATA_CELL_INDEX_HPP (update both ifndef/define and endif comment).
• phlex/core/framework_graph.hpp
  • Rename member std::stack<layer_sentry> levels_; → std::stack<layer_sentry> layers_; (and update uses in file/framework_graph.cpp)
• Decide and apply consistent vocabulary for “layer” vs “cell”
  • Either rename data_cell_index → data_layer_index OR data_layer_hierarchy → data_cell_hierarchy. Pick whichever aligns with the CDR you mentioned and apply consistently across files/comments.
• Run a project-wide grep for “level” to find any remaining occurrences that should be updated (besides intended English uses like “log the level” etc.).

Notes about items I checked but that appear OK:

• Data structures that previously used level_id::hash_type were consistently changed to data_cell_index::hash_type.
• std::hash specialization for data_cell_index is present, satisfying unordered_map and tbb containers that need hashing.
• Tests were updated widely to the new names (most user-facing references changed).

---

Patch 1:

*** Begin Patch
*** Update File: phlex/model/data_cell_index.hpp
@@
-#ifndef PHLEX_MODEL_DATA_CELL_ID_HPP
-#define PHLEX_MODEL_DATA_CELL_ID_HPP
+#ifndef PHLEX_MODEL_DATA_CELL_INDEX_HPP
+#define PHLEX_MODEL_DATA_CELL_INDEX_HPP
@@
-#endif // PHLEX_MODEL_DATA_CELL_ID_HPP
+#endif // PHLEX_MODEL_DATA_CELL_INDEX_HPP
*** End Patch

Patch 2:

*** Begin Patch
*** Update File: phlex/core/framework_graph.hpp
@@
-    message_sender sender_{hierarchy_, multiplexer_, eoms_};
-    std::queue<product_store_ptr> pending_stores_;
-    flush_counters counters_;
-    std::stack<layer_sentry> levels_;
+    message_sender sender_{hierarchy_, multiplexer_, eoms_};
+    std::queue<product_store_ptr> pending_stores_;
+    flush_counters counters_;
+    std::stack<layer_sentry> layers_;
     bool shutdown_{false};
   };
 }
*** End Patch

[greenc-FNAL]

Some items (not all) were addressed by #160.

@knoepfel Please look at remaining checkboxes in the issue description and address as appropriate.

[knoepfel]

Or clearly document the intended difference between “cell” and “layer” if deliberate.

This distinction is documented in the CDR. I don't believe we need to document it more in the code right now.

Because multiple public names changed, any external users must be updated.

Unnecessary as no public release of the code yet exists.

Decide and apply consistent vocabulary for “layer” vs “cell”

• Either rename data_cell_index → data_layer_index OR data_layer_hierarchy → data_cell_hierarchy. Pick whichever aligns with the CDR you mentioned and apply consistently across files/comments.

We should consider at some point whether data-cell hierarchy makes sense. Certainly, data cells fit within a hierarchy, but I'm not sure it's meaningful to express this in the code right now. We can revisit it later.

The following comments were out of scope:

• Several headers now include both data_cell_index.hpp and fwd.hpp (which itself forward-declares data_cell_index). This is harmless but redundant. Not a correctness bug, just minor cleanup possible.
• to_layer(product_query& label) is defined but takes a non-const reference. The equivalent to_name uses a const&. This mirrors original to_family behavior, but consider whether a const overload (or returning const ref) is desired. Not strictly a bug but something to review for API ergonomics.