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

[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 <https://reviews.llvm.org/D108403#2957084>, @aaron.ballman wrote:

> In D108403#2955651 <https://reviews.llvm.org/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