[PATCH] D67294: Register and parse a simplified version of '#pragma omp declare variant'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: include/clang/Basic/Attr.td:3220
+    private:
+      NamedDecl *VecVarNamedDecl;
+
----------------
andwar wrote:
> 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.
But we have to handle templates from the very beginning. This is the thing that would be very hard to support later and we have to think about it in the first patch.
Just parse function identifier as an expression.
Your solution also will break serialization/deserialization since attributes dies not support serilization of the custom fields.


================
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)) {
----------------
andwar wrote:
> 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?
No, don't try to parse function names directly. What about `::fun` format? Or `std::fun`? Your solution does not support it.
I have basic implementation that handles all these cases. Let me commit it, because we need it to support other context selectors. It is very similar to your solution but handles all the problems. We just don't have time to wait, need to support other context selectors.


================
Comment at: lib/Parse/ParseOpenMP.cpp:788
+}
+
 /// Parse clauses for '#pragma omp declare simd'.
----------------
jdoerfert wrote:
> I actually doubt the "vec-var-id" has to be an `tok::identifier` but let's put that discussion on hold for later patches and get it in for the most common case, identifiers.
This is definitely wrong.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67294