-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[llvm] Support building with c++23 #154372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
4c8f57f
49f1a4c
f8ba12a
ddd4519
b48f2db
214772c
b922f74
d62570e
2a29c49
a801696
cafcfad
932c256
bc927ce
c813d7d
827a562
e20c9d2
fe60ac4
c50dede
455d96d
4d244dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
#define LLVM_DEBUGINFO_PDB_PDBSYMBOLFUNC_H | ||
|
||
#include "llvm/DebugInfo/PDB/IPDBRawSymbol.h" | ||
#include "llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The code generated by the macro at line 73:
Results in a unique_ptr to PDBSymbolTypeFunctionSig which is now a constexpr in c++23. Instead of importing the header here, it should also be possible to move the definition of the methods defined by the macro to the PDBSymbolFunc.cpp file where the complete type is known. Since there are so many methods defined with these macros in PDB, I didn't want to break the paradigm by moving the methods and only declaring them here in the header. |
||
#include "llvm/Support/Compiler.h" | ||
|
||
#include "PDBSymbol.h" | ||
|
@@ -21,7 +22,6 @@ namespace pdb { | |
|
||
class PDBSymDumper; | ||
class PDBSymbolData; | ||
class PDBSymbolTypeFunctionSig; | ||
template <typename ChildType> class IPDBEnumChildren; | ||
|
||
class LLVM_ABI PDBSymbolFunc : public PDBSymbol { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,11 @@ | |
|
||
#include "PDBSymbol.h" | ||
#include "PDBTypes.h" | ||
#include "llvm/DebugInfo/PDB/IPDBLineNumber.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This include solves the above error when compiling against the c++23 standard. Similar to the case of |
||
#include "llvm/Support/Compiler.h" | ||
|
||
#include "llvm/DebugInfo/PDB/IPDBRawSymbol.h" | ||
#include "llvm/DebugInfo/PDB/PDBSymbolTypeBuiltin.h" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Similar to the case of #include "llvm/DebugInfo/PDB/IPDBLineNumber.h" above, this include also provides a complete type which is required in the expanded version of a below macro:
|
||
|
||
namespace llvm { | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes seem largely related to the addition of
constexpr
tounique_ptr
in C++23, which caused Clang to require type completeness in more cases.I guess the completeness requirement is somehow superfluous, see #59292. Perhaps we should mention the that issue in the PR description (which will be used for squashed commit message by default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, clang is a bit eager in instantiating the unique_ptr usage in header files with the constexpr property of c++23.
This is related to #5929 as it is a similar error that was then showing up when constexpr changes with vectors and strings that were added in c+20.
With respect to the errors solved in this PR, the first mention I can find of errors due to the instantiation time of constexpr unique_ptrs is found in #74963.
In particular, this comment describes largest problem addressed in this PR: #74963 (comment)
Referencing the above comment is an example of this issue in another project, which appears to be using MCSV. It looks like these changes with respect to unique_ptr in c++23 have broken a popular paradigm of defining some methods in header files use unique_pointers to hold references to imported classes and methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're working around a clang bug here, can we fix clang instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've found in other issues, it seems that clang isn't wrong to be instantiating the unique_ptrs as early as it is. I'll see what else I can find on the topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is another mention of this issue from a different project. The consensus there is that the other compilers are allowing invalid c++ code according to the standard, and that . So rather I'd say that these errors are the result of a bug-fix.
As commented in the linked issue:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this comment it also seems like the clang team does not intend on changing their implementation to avoid such errors:
Originally posted by @zygoloid in #59966
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronBallman Could you please take a look at this? Is this being "Won't Fix" still the current stance of Clang? This seems like a bad footgun for adoption of future C++ standards :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not necessarily arguing against this but I suspect if we do look into this, it won't be a quick fix and we will likely run into a lot of core issues along the way. I don't see this being resolved quickly.
One also has to consider, we eventually need someone to implement reflection and that is likely going to be a large set of refactoring and a core issue generator galore. Likely the refactoring and core issues will kind of all be around the same areas. So we may be pretty resource constrained once this work starts.
BTW Aaron is out for the next month.