fix: C++11 compatibility - replace structured bindings with iterator …#7479
fix: C++11 compatibility - replace structured bindings with iterator …#7479mohanchen wants to merge 1 commit into
Conversation
…access Replace C++17 structured bindings in dspin_lcao.cpp with C++11-compatible iterator syntax to support C++11 compilation standard.
|
I would like to question the premise of this PR rather than the correctness of the local change. Before replacing structured bindings specifically to preserve C++11 compatibility, could we clarify whether C++11 is still an actively supported minimum standard for ABACUS, and which currently supported compilers or platforms require it? If C++11 remains an intentional project requirement, it would be helpful to document the corresponding compiler support policy and ensure that the relevant build configurations are continuously tested under C++11. A compatibility commitment also creates an ongoing maintenance obligation: future contributors may reasonably introduce improvements using newer and now-common C++ features, and without sufficiently broad and strict CI coverage, the resulting incompatibilities may only be discovered later in particular build configurations and require reactive compatibility fixes, which might in turn makes the claimed C++11 compatibility unreliable. Otherwise, it may be worth discussing whether retaining this baseline still provides enough practical benefit to justify avoiding clearer modern C++ constructs and imposing this additional constraint on future contributions. Also, the PR title and description say that the structured binding is replaced with “iterator syntax”, but the new code still uses a range-based "for" loop. It only replaces the structured binding with explicit ".first" and ".second" access. |
|
@Growl1234 Thanks a lot for your thorough review and valuable feedback. |
|
Thank you for clarification, this is really helpful for me! |
|
I would be willing to help improve the testing and enforcement of this requirement. In my view, the most fundamental solution would be a reliable, customize, and project-controlled pre-commit workflow that contributors can run locally. Contributors should be able to detect and correct these problems before submitting a PR, rather than relying on GitHub CI to discover them afterwards. Such a workflow could verify that the relevant code remains valid under C++11, while avoiding overly mechanical checks that reject constructs merely because they are also commonly associated with later standards, provided that they are still valid C++11. It could also provide consistent formatting and other lightweight source checks. The difficult part is introducing this reliably into the existing repository. Previous experiments have shown that enabling all such checks at once can produce an extremely large number of changes across the current codebase, making the resulting patch difficult to review and potentially mixing mechanical cleanup with substantive changes. A gradual approach may therefore be necessary—for example, initially checking only newly modified files, establishing a baseline for existing violations, and tightening the rules incrementally in separate, reviewable steps. I would also prefer not to rely on a GitHub-hosted pre-commit service as the primary implementation. Its functionality and portability are relatively limited, and it can introduce additional integration problems in a project of this size. The authoritative workflow should instead live in the repository, behave consistently both locally and in CI, and remain under the project’s control. |
There was a problem hiding this comment.
Pull request overview
This PR improves C++11 compatibility in the LCAO DeltaSpin operator implementation by removing a C++17 structured binding usage in dspin_lcao.cpp, allowing compilation under a C++11 standard as intended for the project/toolchain.
Changes:
- Replaced a C++17 structured binding over
bi_ad.nlmwith a C++11-compatible range-for over key/value pairs. - Introduced explicit
first/secondextraction for the global orbital index and itsnlmvector.
Comments suppressed due to low confidence (1)
source/source_lcao/module_operator_lcao/dspin_lcao.cpp:589
- The
if (iw_local < 0)guard splits the{ continue; }across two lines ({ continue;then}), which is easy to misread as a missing brace and is inconsistent with the surrounding brace style in this function. Please format it as a normal multi-lineifblock (or keepcontinueon the same line with the closing brace) to avoid confusion during future edits.
if (iw_local < 0) { continue;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…access
Replace C++17 structured bindings in dspin_lcao.cpp with C++11-compatible iterator syntax to support C++11 compilation standard.
Reminder
Linked Issue
Fix #...
Unit Tests and/or Case Tests for my changes
What's changed?
Any changes of core modules? (ignore if not applicable)