- store NestedNameSpecifier & Loc for the qualifiers This information was entirely missing from the AST.
- expose the location information for enum/qualifier/identifier as typeloc (ElaboratedTypeLoc+EnumTypeLoc for now). This allows many traversals/astmatchers etc to handle these generically along with other references. The decl vs type split can help preserve typedef sugar when https://github.com/llvm/llvm-project/issues/57659 is resolved.
- fix the SourceRange of UsingEnumDecl to include 'using'.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I just saw this touches the same code as @urnathan's D134283, specifically Sema::ActOnUsingEnumDeclaration.
I'm happy to wait for that to land and then merge, I think these will play nicely together. The changes to the actual UsingEnumDecl class are the main thing I'd like some feedback on, and those won't be affected.
AFAICT the UsingDecl doesn't capture the NestedNameSpecifier location, is UsingEnumDecl special in some way, or would this information be better placed in BaseUsingDecl? Thanks for spotting the possible mid-air collistion with D134283.
We have NestedNameSpecifierLoc UsingDecl::QualifierLoc already, maybe I'm misunderstanding.
is UsingEnumDecl special in some way, or would this information be better placed in BaseUsingDecl?
It would be possible to unify the two by moving UsingDecl's QualifierLoc into BaseUsingDecl..
I think this comes at the cost of a worse API for UsingEnumDecl: making the qualifier as part of a Typeloc/TypeSourceInfo means that AST consumers with some generic handling for TypeLocs can reuse it. (e.g. the clangd changes in this patch are mostly just testing various things that now work for free).
UsingDecl already has that comparatively awkward API today - it's one of the AST nodes that very often needs to be handled explicitly - unavoidably as "reference to a decl" is not modeled in the AST, and because it may point to an overload set etc anyway.
clang/lib/AST/ASTImporter.cpp | ||
---|---|---|
4868–4870 | This name would be more consistent with getEnumType. |
Not cleanly, as getType already creates the ElaboratedTypeLoc for the qualifier but without the enum keyword. Wrapping it with a second ElaboratedType isn't consistent with how these types are modeled elsewhere in the AST. Plumbing through parameters to make getType() add the elaborated-type-keyword seems disproportionately invasive.
I think probably it's best just to drop the idea that "enum" is part of a tag name (especially since now the name can be a typedef) and keep treating it as an extra SourceLocation instead.
Modelling this as a TypeLoc for code reuse reasons gives me a pause too.
However, I think we still accept this as keeping the clients simpler seems like the right trade-off to me.
Putting information about both typedefs and qualifiers explicitly will make the representation of UsingEnumDecl way too complicated for a seemingly simple and niche feature.
clang-tools-extra/clangd/unittests/SelectionTests.cpp | ||
---|---|---|
554 | Could we also test that checks what happens in presence of typedefs? namespace enums { enum class X { a, b}; using Y = X; } namespace uses { using enum enums::Y; } |
it looks good to me, just a few optional nits.
clang/include/clang/AST/DeclCXX.h | ||
---|---|---|
3618 | nit: we only rename EnumLocation but not the UsingLocation, it is a little wired, I think either we rename both or keep both unchanged. | |
3623 | I think we can probably get rid of the Enum field, since we are storing the EnumType and we can get the EnumDecl by dyn_cast<EnumDecl>(EnumType->getType()->getAsTagDecl()) | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
11857 | nit: adding = nullptr initializer |
Could we also test that checks what happens in presence of typedefs?