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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
