[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/lib/AST/DeclBase.cpp:1222
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+DC = DC->getParent();

erichkeane wrote:
> cor3ntin wrote:
> > Can getParent() be null? If it cam, then this function can return null 
> > which you don't check when you call that function. Or it can't and your 
> > test is not doing anything.
> I don't think it can ever be null, a DC of transparent-context type is always 
> going to have at least a TranslationUnit as a parent.
> 
> I might think an assertion here to replace the `DC &&` part might be worth it.
Thanks for the suggestions! I switched to an assert in 
65bcdeaa15b729dae40190c6a990cd67c12e9f10.


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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added inline comments.



Comment at: clang/lib/AST/DeclBase.cpp:1222
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+DC = DC->getParent();

cor3ntin wrote:
> Can getParent() be null? If it cam, then this function can return null which 
> you don't check when you call that function. Or it can't and your test is not 
> doing anything.
I don't think it can ever be null, a DC of transparent-context type is always 
going to have at least a TranslationUnit as a parent.

I might think an assertion here to replace the `DC &&` part might be worth it.


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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Corentin Jabot via Phabricator via cfe-commits 
cor3ntin added inline comments.



Comment at: clang/lib/AST/DeclBase.cpp:1222
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+DC = DC->getParent();

Can getParent() be null? If it cam, then this function can return null which 
you don't check when you call that function. Or it can't and your test is not 
doing anything.


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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman closed this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Thanks for the reviews! I've committed (with the formatting fix) in 
48f73ee666a264d23716ff6bb671cad836b65ccf 
.


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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

In D108403#2957084 , @aaron.ballman 
wrote:

> In D108403#2955651 , @erichkeane 
> wrote:
>
>> This whole function seems a little suspect, but I don't have a good example 
>> of a place it would break.  Is there no cases where a lookup could result in 
>> the same COUNT but different declaration set? I guess it is more the 
>> question of whether a transparent context can 'lose' a name lookup (perhaps 
>> a case of conflicting names?), then have it added by the local namespace.
>
> I don't believe transparent contexts can lose a name lookup -- they're 
> transparent, so the declarations aren't owned by that context, they're owned 
> by the first non-transparent context they run into (as I understand it, 
> anyway). However, the important bit on this patch is: `lookup()` asserts that 
> you're not trying to call it on a transparent context, so that tells me that 
> we should walk until we find a non-transparent context rather than the 
> current approach of assuming the parent is not transparent.

Right... you're probably right here, the mechanism with 'lookup' just feels 1/2 
too clever.  Anyway, I think I believe that is a battle for another day.  See 
the clang-format warning above, otherwise LGTM.


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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman updated this revision to Diff 367765.
aaron.ballman added a comment.

Updating based on review comments.


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

https://reviews.llvm.org/D108403

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/lib/AST/DeclBase.cpp
  clang/test/Misc/diag-inline-namespace.cpp


Index: clang/test/Misc/diag-inline-namespace.cpp
===
--- clang/test/Misc/diag-inline-namespace.cpp
+++ clang/test/Misc/diag-inline-namespace.cpp
@@ -48,3 +48,14 @@
   T t4; // expected-error {{implicit instantiation of 
undefined template 'N::T'}}
   T t5; // expected-error {{implicit instantiation of 
undefined template 'N::T'}}
 }
+
+namespace dont_crash {
+// A malformed lookup involving inline namespaces in a linkage specification
+// would previous cause an assertion due to the way diagnostics are emitted.
+extern "C++" inline namespace {
+namespace a {
+  a : b // expected-error {{unexpected ':' in nested name specifier; did you 
mean '::'?}} \
+// expected-error {{no type named 'b' in namespace 'dont_crash::a'}}
+} // expected-error {{expected unqualified-id}}
+} // inline namespace
+} // dont_crash
Index: clang/lib/AST/DeclBase.cpp
===
--- clang/lib/AST/DeclBase.cpp
+++ clang/lib/AST/DeclBase.cpp
@@ -1217,6 +1217,13 @@
   return false;
 }
 
+DeclContext *DeclContext::getNonTransparentContext() {
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+DC = DC->getParent();
+  return DC;
+}
+
 DeclContext *DeclContext::getPrimaryContext() {
   switch (getDeclKind()) {
   case Decl::ExternCContext:
Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1997,6 +1997,12 @@
 return const_cast(this)->getNonClosureAncestor();
   }
 
+  // Retrieve the nearest context that is not a transparent context.
+  DeclContext *getNonTransparentContext();
+  const DeclContext* getNonTransparentContext() const {
+return const_cast(this)->getNonTransparentContext();
+  }
+
   /// getPrimaryContext - There may be many different
   /// declarations of the same entity (including forward declarations
   /// of classes, multiple definitions of namespaces, etc.), each with
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -614,7 +614,9 @@
 if (!isInline())
   return false;
 auto X = lookup(Name);
-auto Y = getParent()->lookup(Name);
+// We should not perform a lookup within a transparent context, so find a
+// non-transparent parent context.
+auto Y = getParent()->getNonTransparentContext()->lookup(Name);
 return std::distance(X.begin(), X.end()) ==
   std::distance(Y.begin(), Y.end());
   }


Index: clang/test/Misc/diag-inline-namespace.cpp
===
--- clang/test/Misc/diag-inline-namespace.cpp
+++ clang/test/Misc/diag-inline-namespace.cpp
@@ -48,3 +48,14 @@
   T t4; // expected-error {{implicit instantiation of undefined template 'N::T'}}
   T t5; // expected-error {{implicit instantiation of undefined template 'N::T'}}
 }
+
+namespace dont_crash {
+// A malformed lookup involving inline namespaces in a linkage specification
+// would previous cause an assertion due to the way diagnostics are emitted.
+extern "C++" inline namespace {
+namespace a {
+  a : b // expected-error {{unexpected ':' in nested name specifier; did you mean '::'?}} \
+// expected-error {{no type named 'b' in namespace 'dont_crash::a'}}
+} // expected-error {{expected unqualified-id}}
+} // inline namespace
+} // dont_crash
Index: clang/lib/AST/DeclBase.cpp
===
--- clang/lib/AST/DeclBase.cpp
+++ clang/lib/AST/DeclBase.cpp
@@ -1217,6 +1217,13 @@
   return false;
 }
 
+DeclContext *DeclContext::getNonTransparentContext() {
+  DeclContext *DC = this;
+  while (DC && DC->isTransparentContext())
+DC = DC->getParent();
+  return DC;
+}
+
 DeclContext *DeclContext::getPrimaryContext() {
   switch (getDeclKind()) {
   case Decl::ExternCContext:
Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1997,6 +1997,12 @@
 return const_cast(this)->getNonClosureAncestor();
   }
 
+  // Retrieve the nearest context that is not a transparent context.
+  DeclContext *getNonTransparentContext();
+  const DeclContext* getNonTransparentContext() const {
+return const_cast(this)->getNonTransparentContext();
+  }
+
   /// getPrimaryContext - There may be many different

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-20 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D108403#2955651 , @erichkeane 
wrote:

> This whole function seems a little suspect, but I don't have a good example 
> of a place it would break.  Is there no cases where a lookup could result in 
> the same COUNT but different declaration set? I guess it is more the question 
> of whether a transparent context can 'lose' a name lookup (perhaps a case of 
> conflicting names?), then have it added by the local namespace.

I don't believe transparent contexts can lose a name lookup -- they're 
transparent, so the declarations aren't owned by that context, they're owned by 
the first non-transparent context they run into (as I understand it, anyway). 
However, the important bit on this patch is: `lookup()` asserts that you're not 
trying to call it on a transparent context, so that tells me that we should 
walk until we find a non-transparent context rather than the current approach 
of assuming the parent is not transparent.




Comment at: clang/include/clang/AST/Decl.h:620
+const DeclContext *Parent = getParent();
+while (Parent->isTransparentContext())
+  Parent = Parent->getParent();

erichkeane wrote:
> This loop seems useful enough to be its own function in DeclContext?  I think 
> I remember seeing us do this for a different patch, right?
This pattern does appear in the code base a few places; I could add a new 
helper method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-19 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

This whole function seems a little suspect, but I don't have a good example of 
a place it would break.  Is there no cases where a lookup could result in the 
same COUNT but different declaration set? I guess it is more the question of 
whether a transparent context can 'lose' a name lookup (perhaps a case of 
conflicting names?), then have it added by the local namespace.




Comment at: clang/include/clang/AST/Decl.h:620
+const DeclContext *Parent = getParent();
+while (Parent->isTransparentContext())
+  Parent = Parent->getParent();

This loop seems useful enough to be its own function in DeclContext?  I think I 
remember seeing us do this for a different patch, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108403

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108403: Fix assertion when generating diagnostic for inline namespaces

2021-08-19 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman created this revision.
aaron.ballman added reviewers: erichkeane, rsmith, rjmccall, jyknight.
aaron.ballman requested review of this revision.
Herald added a project: clang.

When calculating the name to display for inline namespaces, we have custom 
logic to try to hide redundant inline namespaces from the diagnostic. 
Calculating these redundancies requires performing a lookup in the parent 
declaration context, but that lookup should not try to look through transparent 
declaration contexts, like linkage specifications. Instead, loop up the 
declaration context chain until we find a non-transparent context and use that 
instead.

This fixes PR49954.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108403

Files:
  clang/include/clang/AST/Decl.h
  clang/test/Misc/diag-inline-namespace.cpp


Index: clang/test/Misc/diag-inline-namespace.cpp
===
--- clang/test/Misc/diag-inline-namespace.cpp
+++ clang/test/Misc/diag-inline-namespace.cpp
@@ -48,3 +48,14 @@
   T t4; // expected-error {{implicit instantiation of 
undefined template 'N::T'}}
   T t5; // expected-error {{implicit instantiation of 
undefined template 'N::T'}}
 }
+
+namespace dont_crash {
+// A malformed lookup involving inline namespaces in a linkage specification
+// would previous cause an assertion due to the way diagnostics are emitted.
+extern "C++" inline namespace {
+namespace a {
+  a : b // expected-error {{unexpected ':' in nested name specifier; did you 
mean '::'?}} \
+// expected-error {{no type named 'b' in namespace 'dont_crash::a'}}
+} // expected-error {{expected unqualified-id}}
+} // inline namespace
+} // dont_crash
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -614,7 +614,12 @@
 if (!isInline())
   return false;
 auto X = lookup(Name);
-auto Y = getParent()->lookup(Name);
+// We should not perform a lookup within a transparent context, so walk
+// up the context chain until we find a non-transparent context.
+const DeclContext *Parent = getParent();
+while (Parent->isTransparentContext())
+  Parent = Parent->getParent();
+auto Y = Parent->lookup(Name);
 return std::distance(X.begin(), X.end()) ==
   std::distance(Y.begin(), Y.end());
   }


Index: clang/test/Misc/diag-inline-namespace.cpp
===
--- clang/test/Misc/diag-inline-namespace.cpp
+++ clang/test/Misc/diag-inline-namespace.cpp
@@ -48,3 +48,14 @@
   T t4; // expected-error {{implicit instantiation of undefined template 'N::T'}}
   T t5; // expected-error {{implicit instantiation of undefined template 'N::T'}}
 }
+
+namespace dont_crash {
+// A malformed lookup involving inline namespaces in a linkage specification
+// would previous cause an assertion due to the way diagnostics are emitted.
+extern "C++" inline namespace {
+namespace a {
+  a : b // expected-error {{unexpected ':' in nested name specifier; did you mean '::'?}} \
+// expected-error {{no type named 'b' in namespace 'dont_crash::a'}}
+} // expected-error {{expected unqualified-id}}
+} // inline namespace
+} // dont_crash
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -614,7 +614,12 @@
 if (!isInline())
   return false;
 auto X = lookup(Name);
-auto Y = getParent()->lookup(Name);
+// We should not perform a lookup within a transparent context, so walk
+// up the context chain until we find a non-transparent context.
+const DeclContext *Parent = getParent();
+while (Parent->isTransparentContext())
+  Parent = Parent->getParent();
+auto Y = Parent->lookup(Name);
 return std::distance(X.begin(), X.end()) ==
   std::distance(Y.begin(), Y.end());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits