jdoerfert added inline comments.
================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); ---------------- hfinkel wrote: > Is there any way in which this name might become visible to users (e.g., in > error messages)? Yes, I think so. One way to trigger it would be to define the same function in the same `omp begin declare variant scope` (or two with the same context). I haven't verified this though. TBH, I'm unsure how bad this actually is in the short term. The original name is still a at the beginning. We should obviously make sure the error message is appropriate eventually, e.g., it de-mangles the name. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:5471 + // variant expression. We allow this to fail in which case we continue with + // the next best variant expression. + Sema::TentativeAnalysisScope Trap(*this); ---------------- hfinkel wrote: > Please provide an example or otherwise explain why this failure behavior is > correct. Tried to improve the comment. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487 + + VMIs.erase(VMIs.begin() + BestIdx); + Exprs.erase(Exprs.begin() + BestIdx); ---------------- hfinkel wrote: > Why is the ordering here useful? Don't you collect all of the variant clauses > from all of the declarations? Can't there be duplicates? (and does the > relative order need always be the same?) Are we effectively supporting here, > as an extension, cases where not all of the declarations have the same set of > variants declared (it loos like we are because there's no break in the `while > (CalleeFnDecl)` loop, but this makes me wonder if that would still have an > odd behavior. > Why is the ordering here useful? I don't think it is ordered. We have a conceptual set and BestIdx determines the which of the set is the best. All others are equal. > Don't you collect all of the variant clauses from all of the declarations? Yes. > Can't there be duplicates? (and does the relative order need always be the > same?) Yes (and no). `getBestVariantMatchForContext` should determine the best regardless of duplicates, we might just try it multiple times if we didn't manage to create a call. > Are we effectively supporting here, as an extension, cases where not all of > the declarations have the same set of variants declared (it loos like we are > because there's no break in the while (CalleeFnDecl) loop, but this makes me > wonder if that would still have an odd behavior. We are supporting that. All declarations are scanned and all variants are collected. What odd behavior do you refer to? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
