andwar added a comment.
I've addressed most of the comments except for those related to templates. I'd
like to clarify as what is the expected behaviour there before proceeding with
implementation.
================
Comment at: include/clang/Basic/Attr.td:3220
+ private:
+ NamedDecl *VecVarNamedDecl;
+
----------------
ABataev wrote:
> This is definitely wrong, especially if we need to handle templates.
What else would you use? Also, we're not handling template-ids as
'variant-func-id' just yet.
================
Comment at: lib/Parse/ParseOpenMP.cpp:779-783
+ auto Identinfo = PP.getIdentifierInfo(VectorVariantId.Ident->getName());
+ LookupResult Lookup(Actions, Identinfo, VectorVariantId.Loc,
+ Sema::LookupOrdinaryName);
+ CXXScopeSpec SS;
+ if (!Actions.LookupParsedName(Lookup, getCurScope(), &SS)) {
----------------
ABataev wrote:
> 1. All this lookup must be performed in Sema.
> 2. How do you want to handle templates?
> 3. Why just do not parse it as an expression? And rely on existing
> parsing/sema analysis for expressions.
1. That's fine, let me move it there.
2. TL;DR We're not handling templates yet.
3. How about switching to `ParseUnqualifiedId` instead? That will still require
going through `LookupParsedName` though.
Re 2. What the interaction between templates and #pragma(s) should be? The C++
standard doesn't specify that, but IMHO mixing '#pragma omp declare variant'
and templates requires that interaction to be clearly defined.
I know that OpenMP 5 allows 'variant-func-id' to be a template-id, but I
couldn't find an example of any attribute that would use C++ template
parameters. Also, isn't parsing of attributes and templates _orthogonal_ in
clang (i.e. happening at completely different points during parsing)?
In any case, we felt that support for 'template-id' could come later. How do
you feel about it?
================
Comment at: lib/Sema/SemaOpenMP.cpp:4886
+Sema::DeclGroupPtrTy Sema::ActOnOpenMPDeclareVariantDirective(
+ DeclGroupPtrTy DG, IdentifierLoc &VectorVariantId, SourceRange SR) {
----------------
ABataev wrote:
> In this patch need to implement just basic checks, no need to create
> attributes, etc. I would suggest to look at the checks for Target
> Multiversioning attribite (see D40819)
This makes sense, but do you reckon that that should be split into separate
functions? I was inspired by the implementation of `declare simd`. From what I
can tell, the only Sema checking for `declare simd` is happening in
`ActOnOpenMPDeclareVariantDirective`. It feels that doing the same for `declare
variant` would be good for consistency. To limit this method to Sema checks, I
will remove the call to `CreateImplicit`.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67294/new/
https://reviews.llvm.org/D67294
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
