[PATCH] D90568: [clang] Add [is|set]Nested methods to NamespaceDecl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Generally LGTM, though I found a few minor things.



================
Comment at: clang/include/clang/AST/Decl.h:546
+
+  enum Flags : unsigned { F_Inline = 1 << 0, F_Nested = 1 << 1 };
+
----------------
I'm not really loving the `F_` prefixes -- the enumerators are already privately scoped to `NamespaceDecl`, is that not sufficient protection?


================
Comment at: clang/lib/AST/DeclCXX.cpp:2888
+      AnonOrFirstNamespaceAndFlags(nullptr,
+                                   F_Inline * Inline | F_Nested * Nested) {
   setPreviousDecl(PrevDecl);
----------------
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.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:602-607
   if (!StdNamespace) {
     StdNamespace = NamespaceDecl::Create(
         getASTContext(), getASTContext().getTranslationUnitDecl(),
         /*Inline*/ false, SourceLocation(), SourceLocation(),
         &getASTContext().Idents.get("std"),
+        /*PrevDecl*/ nullptr, /*Nested*/ false);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90568/new/

https://reviews.llvm.org/D90568