[clang] Improve nested name specifier AST representation #147835
Conversation
This makes sure NestedNameSpecifierLocs don't apply to the replacement TemplateName of SubstTemplate* nodes. Also removes improper name qualification over these Subst Nodes, causing some canonical TemplateNames to not be fully qualified when printed. Since this is a regression introduced in #147835, which was never released, there are no release notes. Fixes #155281
… name (#155347) This fixes a bug in the fast path for the creation of TagTypes from injected class names. The creation of TagTypes has a fast path which, when there is no elaboration, uses storage in the declaration itself for memoizing the resuling type node, instead of using the folding set. This memoizing would fail when the type was created from the injected class name, as we would look for the node in the injected declaration but store it in the non-injected one, so a different type would be created each time. This regression was reported here: #147835 (comment) Since this regression was never released, there are no release notes.
…ected class name (#155347) This fixes a bug in the fast path for the creation of TagTypes from injected class names. The creation of TagTypes has a fast path which, when there is no elaboration, uses storage in the declaration itself for memoizing the resuling type node, instead of using the folding set. This memoizing would fail when the type was created from the injected class name, as we would look for the node in the injected declaration but store it in the non-injected one, so a different type would be created each time. This regression was reported here: llvm/llvm-project#147835 (comment) Since this regression was never released, there are no release notes.
|
Heads up, it seems Rust's bindgen (which uses libclang) is broken after this change (https://crbug.com/440975178). |
notice the edit: we apparently had suppressed -Wmismatched-tags which would have caught this... |
|
I see, so that's unfortunate MSVC mangles the struct / class difference. So MSVC mangles according to the tag kind of the first declaration, clang after this changes now mangles it according to the declaration found by lookup at the point of use. Sounds like if we want strict MSVC bug-for-bug conformance, we should change that to use the first declaration then. |
We don't want bug-for-bug conformance, but we do want to be ABI compatible. So we do have to undo the regression in behavior here. (I think this was covered by tests even, right?) So we'd appreciate a fix 🙂 |
In llvm/llvm-project#147835, Clang's AST changed so that a TagType's decl will not necessarily refer to the type's definition, but to the exact declaration which produced the type. For example, in typedef struct S T; struct S { int x; }; the 'struct S' type would refer to the incomplete type decl in the typedef, causing CompInfo to fail to see the type's definition. This patch inserts a call to use the definition when available. It fixes the original test case in rust-lang#3275 and most of the test failures caused by the Clang change but not all.
In llvm/llvm-project#147835, Clang's AST changed so that a TagType's decl will not necessarily refer to the type's definition, but to the exact declaration which produced the type. For example, in typedef struct S T; struct S { int x; }; the 'struct S' type would refer to the incomplete type decl in the typedef, causing CompInfo to fail to see the type's definition. This patch inserts a call to use the definition when available. It fixes the original test case in #3275 and most of the test failures caused by the Clang change but not all.
In llvm/llvm-project#147835, Clang's AST changed so that a TagType's decl will not necessarily refer to the type's definition, but to the exact declaration which produced the type. For example, in typedef struct S T; struct S { int x; }; the 'struct S' type would refer to the incomplete type decl in the typedef, causing CompInfo to fail to see the type's definition. This patch inserts a call to use the definition when available. It fixes the original test case in #3275 and most of the test failures caused by the Clang change but not all.
No, this patch didn't change any existing mangling tests at all, the behavior in question here was untested. |
This fixes a regression reported here: #147835 (comment) Since this regression was never released, there are no release notes.
…155662) This fixes a regression reported here: #147835 (comment) Since this regression was never released, there are no release notes.
… tagkind (#155662) This fixes a regression reported here: llvm/llvm-project#147835 (comment) Since this regression was never released, there are no release notes.
It really looks like an MSVC bug. If its mangling depends on whether the first (forward-)declaration of a class in a translation unit uses |
|
The MSVC mangling issue is resolved here: #155662 |
This makes the type printer not qualify constructor and destructor names. These are represented as canonical types and the type printer is used, but unlike canonical types which we normally print as fully qualified, the expected behaviour for declaration names is for them to be unqualified. Note that this restores the behaviour pre #147835, but that is still broken for the constructor names of class templates, since in that case the injected class name type is used, but here the type printer is configured to also print the implicit template arguments. No release notes since this regression was never released. Fixes #155537
I guess this is why MSVC has this warning: https://stackoverflow.com/q/468486 |
This makes the type printer not qualify constructor and destructor names. These are represented as canonical types and the type printer is used, but unlike canonical types which we normally print as fully qualified, the expected behaviour for declaration names is for them to be unqualified. Note that this restores the behaviour pre #147835, but that is still broken for the constructor names of class templates, since in that case the injected class name type is used, but here the type printer is configured to also print the implicit template arguments. No release notes since this regression was never released. Fixes #155537
|
I mean, it's an MSVC bug in the sense that it's non-conformant behavior, but it's non-conformant behavior that has long since become a cornerstone of the MSVC ABI, so the bugginess is no longer relevant. We are obliged to maintain compatibility with it if we want to say we implement the ABI correctly. |
|
Yeah, the not-to-break-ABI consideration is obviously the only reason why they haven't fixed it so far. But this makes dangerous to mix declarations with different tag kinds despite it is allowed by the standard, and hence MSVC has a warning on that as @kimgr has noticed. So probably, there isn't much sense for strict correspondence in that case because it might be said that this is user's code problem... Anyway, it is fixed now. |
|
FWIW, clang has a warning like that too, it's |
In llvm/llvm-project#147835, Clang's AST changed so that a TagType's decl will not necessarily refer to the type's definition, but to the exact declaration which produced the type. For example, in typedef struct S T; struct S { int x; }; the 'struct S' type would refer to the incomplete type decl in the typedef, causing CompInfo to fail to see the type's definition. This patch inserts a call to use the definition when available. It fixes the original test case in #3275 and most of the test failures caused by the Clang change but not all.
This is a major change on how we represent nested name qualifications in the AST.
This patch offers a great performance benefit.
It greatly improves compilation time for stdexec. For one datapoint, for
test_on2.cppin that project, which is the slowest compiling test, this patch improves-ccompilation time by about 7.2%, with the-fsyntax-onlyimprovement being at ~12%.This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and will reduce the performance impact of template specialization resugaring when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the reason is that I started by the nested name specifier part, before the ElaboratedType part, but that had a huge performance downside, as ElaboratedType is a big performance hog. I didn't have the steam to go back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove ElaboratedType in one go, versus removing it from one type at a time, as that would present much more churn to the users. Also, the nested name specifier having a different API avoids missing changes related to how prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
clang/include/clang/ASTandclang/lib/AST, with also important changes inclang/lib/Sema/TreeTransform.h.The rest and bulk of the changes are mostly consequences of the changes in API.
PS: TagType::getDecl is renamed to
getOriginalDeclin this patch, just for easier to rebasing. I plan to rename it back after this lands.Fixes #136624
Fixes #43179
Fixes #68670
Fixes #92757