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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:542
+    N = Decls.size();
+  }
+
----------------
john.brawn wrote:
> rjmccall wrote:
> > 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.
> I think that delaying the removal until after the main loop would just complicate things, as then in the main loop we would have to check each index to see if it's something we're going to later remove. I can adjust it to do the erasing more like it's done in the main loop though, i.e. move the erased element to the end and decrement N, so the call to Decls.truncate will remove it. We can't use bitset though, as that takes the size of the bitset (which in this case would be the number of decls) as a template parameter.
llvm::BitVector should work for this. 


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

https://reviews.llvm.org/D154503