hfinkel 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()); ---------------- jdoerfert wrote: > 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. I think that we have to consider diagnostic quality as a first-class citizen. There are a few different ways that we might approach this. A minimal diff from what you have might be: 1. Replace the "." above with ".ompvariant." 2. Add logic to FunctionDecl::getNameForDiagnostic (or NamedDecl::getNameForDiagnostic) that recognizes that magic string in the name and alters the printing to do something intelligible for the user with the name. (I think that (1) is a good idea anyway). Another way would be to add some first-class property to the function and then leave the mangling to CodeGen. Are we mangling these in Sema in other places too? Also, these can have external linkage, right? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487 + + VMIs.erase(VMIs.begin() + BestIdx); + Exprs.erase(Exprs.begin() + BestIdx); ---------------- jdoerfert wrote: > 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? > Makes sense, thanks. 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
