This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add [is|set]Nested methods to NamespaceDecl
ClosedPublic

Authored by njames93 on Nov 1 2020, 11:07 AM.

Details

Summary

Adds support for NamespaceDecl to inform if its part of a nested namespace.
This flag only corresponds to the inner namespaces in a nested namespace declaration.
In this example:
namespace <X>::<Y>::<Z> {}
Only <Y> and <Z> will be classified as nested.

This flag isn't meant for assisting in building the AST, more for static analysis and refactorings.

Diff Detail

Event Timeline

njames93 created this revision.Nov 1 2020, 11:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
njames93 requested review of this revision.Nov 1 2020, 11:07 AM

Just a few minor nits. I'll leave the approving to the other reviewers.

clang/lib/AST/TextNodeDumper.cpp
1935

Can you also modify JSONNodeDumper::VisitNamespaceDecl ?

clang/lib/Sema/SemaDeclCXX.cpp
11502

Please change false to /*Nested=*/false.

shafik added inline comments.Nov 2 2020, 4:13 PM
clang/lib/AST/ASTContext.cpp
8766–8770

As noted elsewhere instead of just false please use /*Nested*/ false

Also a little below.

njames93 updated this revision to Diff 302437.Nov 2 2020, 5:09 PM

Added JsonNodeDumper and argument comments.

Added JsonNodeDumper and argument comments.

Thanks, I've no further comments.

njames93 marked 3 inline comments as done.Nov 25 2020, 10:39 AM
njames93 updated this revision to Diff 473444.Nov 5 2022, 10:57 AM

Rebased
Changed logic so inner namespaces are marked as nester(which makes much more sense imo)

Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2022, 10:57 AM
njames93 edited the summary of this revision. (Show Details)Nov 5 2022, 10:59 AM
njames93 added a reviewer: aaron.ballman.
njames93 updated this revision to Diff 473457.Nov 5 2022, 1:34 PM

Fix clang-format

aaron.ballman accepted this revision.Nov 7 2022, 12:36 PM

Generally LGTM, though I found a few minor things.

clang/include/clang/AST/Decl.h
546

I'm not really loving the F_ prefixes -- the enumerators are already privately scoped to NamespaceDecl, is that not sufficient protection?

clang/lib/AST/DeclCXX.cpp
2888

I love it and hate it in equal measures; I think multiplying a bool and an enumerator together is just a bit too clever. How about:

unsigned Flags = 0;
if (Inline) Flags |= F_Inline;
if (Nested) Flags |= F_Nested;
AnonOrFirstNamespaceAndFlags(nullptr, Flags);

It's longer, but I think it's also more clear.

clang/lib/AST/ItaniumMangle.cpp
602–607

Not your problem, but I'm curious if you agree: we should probably have a helper method for getting the std namespace (and creating it if necessary) so we don't do this dance in at least three different places.

This revision is now accepted and ready to land.Nov 7 2022, 12:36 PM
njames93 updated this revision to Diff 473810.Nov 7 2022, 3:14 PM
njames93 marked an inline comment as done.

Clean up initializer.

shafik added inline comments.Nov 10 2022, 2:51 PM
clang/lib/AST/ASTContext.cpp
8767–8770

Overriding my old comment from a while ago,

8957–8959
clang/lib/AST/ItaniumMangle.cpp
605–607
clang/lib/Sema/HLSLExternalSemaSource.cpp
386
njames93 updated this revision to Diff 474632.Nov 10 2022, 4:36 PM
njames93 marked 3 inline comments as done.

Fix comments.

clang/include/clang/AST/Decl.h
546

I think just having it named as Inline and Nested is going to confuse things even more, BlockDecl seems to use flag_is<FLAG> for its packing, and I think that is even worse. WDYT

njames93 added inline comments.Nov 10 2022, 4:41 PM
clang/test/AST/ast-dump-decl.cpp
1–823

This is confusing me, I'm not sure exactly why this file appears to have been complete changed when it doesn't appear to, CL/LF maybe?

aaron.ballman added inline comments.Nov 16 2022, 11:51 AM
clang/include/clang/AST/Decl.h
546

Eh, I don't feel super strongly, but if you want to force there to be a qualified name, then I'd hoist it out of NamespaceDecl and make a scoped enum so you can write Namespace::Inline or Namespace::Nested.

clang/test/AST/ast-dump-decl.cpp
1–823

Yeah this looks like a line ending change slipped in because Phab is showing green/red on lines with no visible modifications. FWIW, my local copy seems to be using UNIX line endings and not Windows line endings.

njames93 updated this revision to Diff 477467.Nov 23 2022, 4:56 AM

Hopefully fixed line endings

This revision was landed with ongoing or failed builds.Nov 24 2022, 4:45 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 24 2022, 4:58 AM

This breaks the build: http://45.33.8.238/linux/92294/step_4.txt

Please take a look and revert for now if it takes a while to fix.

This breaks the build: http://45.33.8.238/linux/92294/step_4.txt

Please take a look and revert for now if it takes a while to fix.

Apologies it took 2 times, It should all be good now, lmk if there's any other issues.

My bots look happy at least. Thanks for the quick fix!