Eliminate ambiguity in text file ending requirements by greenc-FNAL · Pull Request #308 · Framework-R-D/phlex

greenc-FNAL · 2026-02-10T16:02:42Z

Further improve text-file ending requirements
Eliminate ambiguity in text file ending requirements (Eliminate ambiguity in text file ending requirements #307)
Add phlex::identifier (Add phlex::identifier #288)
Enable coverage monitoring for non-test Python modules (Enable coverage monitoring for non-test Python modules #304)
Rename product_store::id() to product_store::index() (Rename product_store::id() to product_store::index() #303)

greenc-FNAL · 2026-02-10T16:06:41Z

Review the full CodeQL report for details.

Copilot

Pull request overview

This PR bundles several unrelated updates across repo tooling, C++ core/model APIs, and tests—most notably clarifying repository text-file EOF formatting rules, introducing phlex::experimental::identifier, renaming product_store::id() to product_store::index(), and tightening Python coverage configuration.

Changes:

Clarify .github/copilot-instructions.md file-ending + trailing-whitespace rules with explicit examples.
Introduce phlex::experimental::identifier (model + JSON conversion hook) and add a corresponding test.
Rename product_store::id() → product_store::index() and propagate across core, tests, and form module; wire Python coverage config + CI deps.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`test/python/check_sys_path.py`	Minor formatting/line-wrapping adjustments for the sys.path checker.
`test/python/CMakeLists.txt`	Adds `--cov-config` so pytest-cov uses the repo’s `.coveragerc`.
`test/python/.gitignore`	Ignores Python coverage artifacts.
`test/python/.coveragerc`	Configures coverage to include non-test modules while omitting test files.
`test/products_for_output.hpp`	Updates logging to use `store.index()` after API rename.
`test/product_store.cpp`	Updates tests to use `index()` when constructing child stores.
`test/identifier.cpp`	Adds a new test executable for `identifier` behaviors + JSON extraction helper.
`test/CMakeLists.txt`	Registers the new `identifier` test.
`phlex/model/product_store.hpp`	Renames `id()` accessor to `index()`.
`phlex/model/product_store.cpp`	Implements `index()` and updates `more_derived` to use it.
`phlex/model/identifier.hpp`	Adds the new `identifier` type, UDLs, and hashing/formatting hooks.
`phlex/model/identifier.cpp`	Implements hashing, comparisons, and UDL definitions.
`phlex/model/CMakeLists.txt`	Builds/installs the new `identifier` source/header.
`phlex/core/store_counters.cpp`	Updates store hashing to use `store->index()`.
`phlex/core/multiplexer.cpp`	Updates layer/parent logic and debug output to use `index()`.
`phlex/core/message_sender.cpp`	Updates message bookkeeping to key off `store->index()`.
`phlex/core/message.cpp`	Updates depth comparison to use `index()`.
`phlex/core/framework_graph.cpp`	Updates depth/counter extraction to use `index()`.
`phlex/core/declared_unfold.hpp`	Updates unfolding logic to use `index()` (hashes, parent IDs, flush handling).
`phlex/core/declared_unfold.cpp`	Updates generator to derive children from `index()`; updates debug output.
`phlex/core/declared_transform.hpp`	Updates transform caching/counters to use `index()`.
`phlex/core/declared_transform.cpp`	Updates debug output to use `index()`.
`phlex/core/declared_provider.hpp`	Updates provider caching and invokes to use `index()`.
`phlex/core/declared_provider.cpp`	Updates debug output to use `index()`.
`phlex/core/declared_predicate.hpp`	Updates predicate caching/flags to use `index()`.
`phlex/core/declared_observer.hpp`	Updates observer caching/flags to use `index()`.
`phlex/core/declared_fold.hpp`	Updates fold partition logic and result lookup to use `index()`.
`phlex/configuration.hpp`	Declares JSON `tag_invoke` for `identifier` and a new `detail::value_if_exists` returning `optional<identifier>`.
`phlex/configuration.cpp`	Implements `detail::value_if_exists` and `tag_invoke` for `identifier`.
`form/form_module.cpp`	Updates form output module to use `store.index()->to_string()`.
`ci/spack.yaml`	Adds `py-pytest-cov` to CI container inputs.
`Modules/private/CreateCoverageTargets.cmake`	Adds `--cov-config` for Python coverage target.
`.github/copilot-instructions.md`	Rewrites EOF/trailing whitespace requirements to be explicit and unambiguous.

Copilot · 2026-02-10T16:09:05Z

phlex/configuration.cpp

    if (!obj.contains(parameter)) {
      return std::nullopt;
    }
    auto const& val = obj.at(parameter);


value_if_exists now attempts value_to<identifier> whenever the key exists, even if the JSON value is null. Previously, null was treated as "not provided" (returning std::nullopt), which matches the function’s return type. Consider restoring the null => std::nullopt behavior (e.g., check val.is_null() / kind) to avoid unexpected exceptions when configs explicitly set a field to null.

Suggested change

auto const& val = obj.at(parameter);

auto const& val = obj.at(parameter);

if (val.is_null()) {

return std::nullopt;

}

.github/copilot-instructions.md

Copilot · 2026-02-10T16:09:06Z

phlex/model/identifier.hpp

+    // Conversion to std::string_view
+    explicit operator std::string_view() const noexcept;
+
+    bool operator==(identifier const& rhs) const noexcept;
+    std::strong_ordering operator<=>(identifier const& rhs) const noexcept;
+
+    // Comparison operators with _id queries
+    friend bool operator==(identifier const& lhs, identifier_query rhs);
+    friend std::strong_ordering operator<=>(identifier const& lhs, identifier_query rhs);
+    friend std::hash<identifier>;
+
+  private:
+    std::string content_;
+    std::uint64_t hash_;
+  };
+
+  // Identifier UDL
+  namespace literals {
+    identifier operator""_id(char const* lit, std::size_t len);
+    identifier_query operator""_idq(char const* lit, std::size_t len);
+  }


identifier.hpp uses std::strong_ordering, std::hash, and std::size_t but does not include the standard headers that define them (typically <compare>, <functional>, and <cstddef>). This can lead to non-portable builds that fail depending on transitive includes; please add the required headers explicitly.

Copilot · 2026-02-10T16:09:06Z

phlex/model/identifier.hpp

+template <>
+struct std::hash<phlex::experimental::identifier> {
+  std::size_t operator()(phlex::experimental::identifier const& id) const noexcept
+  {
+    return id.hash_;
+  }
+};
+


The std::hash specialization for phlex::experimental::identifier is declared at global scope as template <> struct std::hash<...>. Explicit specializations must be declared within the namespace of the primary template (i.e., inside namespace std { ... })—see how data_cell_index.hpp does it. Please move this specialization into namespace std to avoid undefined/ill-formed behavior across compilers.

Suggested change

template <>

struct std::hash<phlex::experimental::identifier> {

std::size_t operator()(phlex::experimental::identifier const& id) const noexcept

{

return id.hash_;

}

};

namespace std {

template <>

struct hash<phlex::experimental::identifier> {

std::size_t operator()(phlex::experimental::identifier const& id) const noexcept

{

return id.hash_;

}

};

} // namespace std

Copilot · 2026-02-10T16:09:07Z

phlex/model/identifier.hpp

+    std::uint64_t hash;
+  };
+
+  /// Carries around the string itself (as a shared_ptr to string to make copies lighter)


The class comment says the string is stored "as a shared_ptr to string to make copies lighter", but the implementation stores std::string content_ by value. Please update the comment to match the actual design, or adjust the implementation if the shared ownership behavior is required.

Suggested change

/// Carries around the string itself (as a shared_ptr to string to make copies lighter)

/// Carries around the string itself, stored by value,

Copilot · 2026-02-10T16:09:07Z

phlex/model/identifier.cpp

+  bool operator==(identifier const& lhs, identifier_query rhs) { return lhs.hash_ == rhs.hash; }
+  std::strong_ordering operator<=>(identifier const& lhs, identifier_query rhs)
+  {
+    return lhs.hash_ <=> rhs.hash;
+  }


operator==(identifier, identifier_query) compares only the 64-bit hash. This makes equality with _idq potentially report true for different strings on hash collision, which breaks the usual expectation that == is exact. Consider storing the literal string/length in identifier_query and comparing content_ when the hash matches, or clearly document that _idq comparisons are probabilistic and should not be used for correctness-critical checks.

Copilot · 2026-02-10T16:09:07Z

phlex/model/identifier.cpp

+  std::uint64_t identifier::hash_string(std::string_view str)
+  {
+    // Hash quality is very important here, since comparisons are done using only the hash
+    using namespace boost::hash2;
+    xxhash_64 h;
+    hash_append(h, {}, str);
+    return h.result();


This comment says "comparisons are done using only the hash", but the implementation falls back to comparing content_ when the hashes match (both in operator== and <=>). Please update the comment to reflect the actual behavior so it doesn't mislead future maintainers.

knoepfel · 2026-02-10T16:19:40Z

@greenc-FNAL, can you rebase this off of main so that the superfluous changes are not included?

- Copilot managed to misinterpret the existing instructions—again; now fixed.

* Initial plan * Clarify text file ending requirements to eliminate ambiguity Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Completely rewrite text formatting instructions for absolute clarity Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

greenc-FNAL requested review from Copilot and knoepfel February 10, 2026 16:02

Copilot started reviewing on behalf of greenc-FNAL February 10, 2026 16:03 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

greenc-FNAL and others added 2 commits February 10, 2026 11:55

Further improve text-file ending requirements

f35c4bb

- Copilot managed to misinterpret the existing instructions—again; now fixed.

greenc-FNAL force-pushed the maintenance/more-newline-shenanigans branch from c440e46 to b7afc50 Compare February 10, 2026 17:55

knoepfel approved these changes Feb 10, 2026

View reviewed changes

knoepfel merged commit d593b0e into main Feb 10, 2026
45 checks passed

knoepfel deleted the maintenance/more-newline-shenanigans branch February 10, 2026 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate ambiguity in text file ending requirements#308

Eliminate ambiguity in text file ending requirements#308
knoepfel merged 2 commits intomainfrom
maintenance/more-newline-shenanigans

greenc-FNAL commented Feb 10, 2026

Uh oh!

greenc-FNAL commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

knoepfel commented Feb 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// Carries around the string itself (as a shared_ptr to string to make copies lighter)
	/// Carries around the string itself, stored by value,

Conversation

greenc-FNAL commented Feb 10, 2026

Uh oh!

greenc-FNAL commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

knoepfel commented Feb 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants