Skip to content

fix: C++11 compatibility - replace structured bindings with iterator …#7479

Open
mohanchen wants to merge 1 commit into
deepmodeling:developfrom
mohanchen:20260617-hotfix
Open

fix: C++11 compatibility - replace structured bindings with iterator …#7479
mohanchen wants to merge 1 commit into
deepmodeling:developfrom
mohanchen:20260617-hotfix

Conversation

@mohanchen

Copy link
Copy Markdown
Collaborator

…access

Replace C++17 structured bindings in dspin_lcao.cpp with C++11-compatible iterator syntax to support C++11 compilation standard.

Reminder

  • Have you linked an issue with this pull request?
  • Have you added adequate unit tests and/or case tests for your pull request?
  • Have you noticed possible changes of behavior below or in the linked issue?
  • Have you explained the changes of codes in core modules of ESolver, HSolver, ElecState, Hamilt, Operator or Psi? (ignore if not applicable)

Linked Issue

Fix #...

Unit Tests and/or Case Tests for my changes

  • A unit test is added for each new feature or bug fix.

What's changed?

  • Example: My changes might affect the performance of the application under certain conditions, and I have tested the impact on various scenarios...

Any changes of core modules? (ignore if not applicable)

  • Example: I have added a new virtual function in the esolver base class in order to ...

…access

Replace C++17 structured bindings in dspin_lcao.cpp with C++11-compatible iterator syntax to support C++11 compilation standard.
@mohanchen mohanchen requested review from dyzheng and ieiue June 17, 2026 04:26
@mohanchen mohanchen added Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS Refactor Refactor ABACUS codes labels Jun 17, 2026
@Growl1234

Growl1234 commented Jun 17, 2026

Copy link
Copy Markdown

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.

@mohanchen

mohanchen commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator Author

@Growl1234 Thanks a lot for your thorough review and valuable feedback.
First of all, to give you a clear confirmation: C++11 remains the minimum supported C++ standard for the ABACUS project at present.
Second, we would highly appreciate it if you are willing to help improve our relevant documentation and CI test configurations for C++11 compatibility. Any contributions on this front are more than welcome.
Also, thank you very much for pointing out the inaccurate wording in the PR title and description. Your correction will definitely help future contributors avoid similar confusion.
I would like to elaborate on your question regarding which maintained compilers and platforms require C++11 support. I can state this responsibly: we are actively porting ABACUS to multiple domestic computing platforms. Removing the C++11 baseline would severely block all these porting efforts. Without smooth progress on platform adaptation, the project’s funding will be at serious risk. This compatibility constraint is a hard, non-negotiable bottom line critical to the survival of ABACUS for the time being.
I only share these background details because I can see you care deeply about the project. I realize I may have gone into too much detail, and perhaps it would have been better to keep my response more concise.

@Growl1234

Copy link
Copy Markdown

Thank you for clarification, this is really helpful for me!

@Growl1234

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.nlm with a C++11-compatible range-for over key/value pairs.
  • Introduced explicit first/second extraction for the global orbital index and its nlm vector.
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-line if block (or keep continue on 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.

@mohanchen mohanchen requested review from dzzz2001 and removed request for dzzz2001 June 18, 2026 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Compile & CICD & Docs & Dependencies Issues related to compiling ABACUS Refactor Refactor ABACUS codes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants