john.brawn added inline comments.

================
Comment at: clang/lib/Sema/SemaLookup.cpp:542
+    N = Decls.size();
+  }
+
----------------
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%.


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

https://reviews.llvm.org/D154503

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

Reply via email to