[Do Not Merge] Default Palace to C++20#654
Draft
phdum-a wants to merge 11 commits into
Draft
Conversation
phdum-a
changed the title
The requires conversion in postoperator/postoperatorcsv left three enable_if_t overloads in units.hpp (Dimensionalize/Nondimensionalize for Vector/ComplexVector) and a now-incorrect TODO(C++20) comment in postoperatorcsv.hpp whose code already uses requires. Convert the remaining overloads and drop the stale comment so no SFINAE remains. For Nondimensionalize this also resolves a latent ambiguity: a satisfied requires constraint makes the Vector overload preferred over the unconstrained generic via C++20 constraint partial-ordering.
Now that C++20 is required, replace pre-C++20 workarounds:
- !s.compare(0, n, "x") / s.rfind("x", 0) == 0 / s.find("x") == 0 /
s.substr(0, n) == prefix -> s.starts_with("x") (meshio, jsonschema,
memoryreporting, ceed). The substr form also drops a heap allocation.
- container.erase(std::remove[_if](...), container.end()) ->
std::erase / std::erase_if (basesolver, geodata, meshio).
Pure readability/idiom changes; behavior is identical.
M_PI is a POSIX extension, not standard C++ (it needs _USE_MATH_DEFINES on MSVC and the right feature-test macros elsewhere). C++20 provides the portable std::numbers::pi in <numbers>. Replace all 47 uses across the codebase and add the <numbers> include where needed.
C++20 adds contains() to associative containers. Replace .count(k) > 0 / == 0 and .find(k) != .end() checks (where the iterator is not subsequently used) with the clearer .contains(k) in the libCEED attribute lookups (mesh) and submesh boundary remapping (geodata).
Clarity-focused cleanups, no behavior change: - container.size() > 0 / == 0 -> !container.empty() / container.empty() for std::vector/string/map/set call sites (left fmt::memory_buffer and the custom FarFieldData::size() alone -- they are not containers). - Hand-written iterator loops (auto it = c.begin(); it != c.end(); ++it) using only it->first/second -> range-for with structured bindings (mesh, materialoperator, configfile periodic-pair parsing). - An erase-while-iterating loop over an unordered_map -> std::erase_if (geodata crack seam-edge pruning).
These comments deferred switching three non-owning reference/pointer members to std::reference_wrapper until C++20. C++20 is now the baseline, and the conversion is declined: all three enclosing classes are single-use local objects that are never assigned or rebound, so reference_wrapper's only advantage (assignability) is unneeded, while it would force .get() at every access site (no operator->). Drop the misleading markers; keep the descriptive 'not owned' comments.
hughcars
force-pushed
the
phdum/cpp20
branch
2 times, most recently
from
a5254d4 to
0d5460a
Compare
The 4 ARM+Clang `build-and-test-linux` jobs failed because Ubuntu jammy's distro `clang` package resolves to clang-14, which has a real constraint-checking bug: explicit instantiation of a class template also (incorrectly) instantiates the bodies of `requires`-constrained non-template members for specializations where the constraint is not satisfied (llvm/llvm-project#52625, #46029). This produced errors like "invalid reference to function 'SetVGridFunction': constraints not satisfied" in postoperator.cpp/postoperatorcsv.cpp. Rather than work around this compiler bug in the C++ source (adding a defaulted-dummy-template-parameter indirection that shouldn't be needed), fix it at the CI toolchain level: install clang-19 (and lld-19) from apt.llvm.org, the same repo and version style.yml already uses for clang-format-19, instead of the distro default. Confirmed empirically (compiling the exact failing pattern) that clang-15 through clang-21 all handle this correctly; clang-19 is pinned for consistency with the rest of the project's toolchains (style.yml's clang-format-19, and the self-hosted palace-ci composite action's `llvm-version: '19'`, which is why build-test-arm64-llvm/build-test-x64-llvm never hit this in the first place). `-fuse-ld=lld` is also used for these builds later in the same job, so add an `ld.lld` alternative pointing at the versioned `ld.lld-19` binary alongside the `clang`/`clang++` alternatives, so the linker resolution isn't left to chance. The postoperator.cpp/postoperatorcsv.cpp/postoperator.hpp/postoperatorcsv.hpp `requires` clauses are back to their original, direct form (no workaround).
Bind untrusted github.* and inputs.* context values to env variables and reference them as shell variables in run: blocks, instead of interpolating them directly into the script text. Prevents the script-injection class flagged by AppSec (Talos). Follows GitHub's security-hardening guidance and the approach of #787.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch moves Palace to C++20 and makes some code simplifications:
Should not be merged until we have a "last C++17" release.
Requires #653 to be merged into main first to fix C++20 compilation issues.