Skip to content

fix: compilation with recent clang#527

Merged
ilfreddy merged 4 commits intoMRChemSoft:masterfrom
robertodr:fix-clang-compilation
Dec 8, 2025
Merged

fix: compilation with recent clang#527
ilfreddy merged 4 commits intoMRChemSoft:masterfrom
robertodr:fix-clang-compilation

Conversation

@robertodr
Copy link
Contributor

No description provided.

@msnik1999
Copy link
Contributor

I have two fixes, which allow compilation on MacOS:

  1. include "qmfunctions/Orbital.h" in Accelerator.h
  2. a small fix for MRCPP I will include in this MRCPP PR

@robertodr
Copy link
Contributor Author

Can you push 1 directly to this PR?

needs to be close to the class definition
@robertodr
Copy link
Contributor Author

🤔 actually, point 1 sounds suspicious. qmfunctions_fwd.h is imported, which forward-declares that stuff, and given how the stuff in Orbital.h is used in Accelerator.h, it should be enough...

@robertodr
Copy link
Contributor Author

I think the problem was that in Accelerator.cpp, the include of Accelerator.h was not coming first. That's a mistake (and I think it's made in many places). In general, when you split the code for a class in header + source, the header should be included first thing in the corresponding source file.

@msnik1999
Copy link
Contributor

msnik1999 commented Dec 8, 2025

No, the definition of std::deque<OrbitalVector> orbitals; in Accelerator.h already needs the Orbital.h include.
It won't compile without that incude.
A lot of changes have been made to std::deque in newer C++-versions (cppreference)

@msnik1999
Copy link
Contributor

🤔 actually, point 1 sounds suspicious. qmfunctions_fwd.h is imported, which forward-declares that stuff, and given how the stuff in Orbital.h is used in Accelerator.h, it should be enough...

Even when I make the forward declaration in the file Accelerator.h, it doesn't compile. I also only know this to work for smart pointers? I've personally never seen it for vectors (or similar).

@robertodr
Copy link
Contributor Author

You're right. But it's a total mystery why things worked before 🤷

@msnik1999
Copy link
Contributor

You're right. But it's a total mystery why things worked before 🤷

In the past, we used different compilers. Now, we are dependent on newer compilers due to the inclusion of the C++-17 feature std::filesystem added about a year ago. Because we don't declare our C++-version anywhere in our CMake-structure (as far as I know), the newer compilers probably use newer C++-standards.

@robertodr
Copy link
Contributor Author

We do fix the version of the standard to use: https://github.com/MRChemSoft/mrchem/blob/master/cmake/compiler_flags/CXXFlags.cmake#L16

@robertodr
Copy link
Contributor Author

Why was I able to reproduce one compilation issue with Clang 21 on Linux, but not all of the ones you encountered?

@msnik1999
Copy link
Contributor

We do fix the version of the standard to use: https://github.com/MRChemSoft/mrchem/blob/master/cmake/compiler_flags/CXXFlags.cmake#L16

Oh, haven't seen this before. But still, we need newer compilers and therefore introduce new errors and a lot of warnings.

@msnik1999
Copy link
Contributor

Why was I able to reproduce one compilation issue with Clang 21 on Linux, but not all of the ones you encountered?

ARM on macOS works in mysterious ways ...

@robertodr
Copy link
Contributor Author

well, thanks for keeping an eye out for new errors and warnings 🙌

@ilfreddy ilfreddy merged commit cf9db17 into MRChemSoft:master Dec 8, 2025
6 checks passed
@robertodr robertodr deleted the fix-clang-compilation branch December 8, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants