[PATCH] D154503: [Sema] Fix handling of functions that hide classes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:542
+    N = Decls.size();
+  }
+
----------------
john.brawn wrote:
> rjmccall wrote:
> > This is going to fire on every single ordinary lookup that finds multiple declarations, right?  I haven't fully internalized the issue you're solving here, but this is a very performance-sensitive path in the compiler; there's a reason this code is written to very carefully only do extra work when we've detected in the loop below that we're in a hidden-declarations situation.  Is there any way we can restore that basic structure?
> Test4 in the added tests is an example of why we can't wait until after the main loop. The `using A::X` in namespace D is eliminated by the UniqueResult check, so the check for a declaration being hidden can only see the using declarations in namespace C. We also can't do it in the loop itself, as then we can't handle Test5: at the time we process the `using A::X` in namespace D it looks like it may cause ambiguity, but it's later hidden by the `using B::X` in the same namespace which we haven't yet processed.
> 
> I have adjusted it though so the nested loop and erasing of decls only happens when we both have things that hide and things that can be hidden. Doing some quick testing of compiling SemaOpenMP.cpp (the largest file in clang), LookupResult::resolveKind is called 75318 times, of which 75283 calls have HideTags=true, of which 56 meet this condition, i.e. 0.07%.
Okay, I can see why you need to not mix tag-hiding with the removal of duplicates.  However, I think you can maintain the current structure by delaying the actual removal of declarations until after the main loop; have the loop build up a set of indices to remove instead.  (Also, you can keep this set as a bitset instead of a `std::set<unsigned>`.)

It's a shame that the hiding algorithm has to check every other declaration in the lookup in case they're from different scopes.  I guess to avoid that we'd have to do the filtering immediately when we collect the declarations from a particular DC.


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

https://reviews.llvm.org/D154503