[Openmp-commits] [PATCH] D105648: [OpenMP] Support OpenMP 5.1 attributes

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2670-2676
+
+  // The next token may be an OpenMP pragma annotation token. That would
+  // normally be handled from ParseCXXClassMemberDeclarationWithPragmas, but in
+  // this case, it came from an *attribute* rather than a pragma. Handle it now.
+  if (Tok.is(tok::annot_pragma_openmp_from_attr))
+    return ParseOpenMPDeclarativeDirectiveWithExtDecl(AS, attrs);
+
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > jdoerfert wrote:
> > > cor3ntin wrote:
> > > > The comment raises 2 questions: should it be called `annot_openmp_attr` instead? Does the comment describe what this does?
> > > > I imagine long terms attributes might be the ""normal"" scenario?
> > > > I imagine long terms attributes might be the ""normal"" scenario?
> > > 
> > > We can't assume that (C) and for C++ only time will tell.
> > FWIW, C23 is getting C++ style attributes as well, so we MIGHT be able to make that assumption some day :D
> > 
> > That said, the fact that these are called "PRAGMA_ANNOTATION" in TokenKinds.def seems misnamed to me anymore, which I think is the confusion.  It is a little strange that the `annot` is added automatically, but the `pragma` isn't... 
> > 
> > Either way, I think it is debatable what the `pragma` in these relates to.  My opinion is that it applies to the syntax (since the rest are #pragma SOMETHING), not that it is a `PRAGMA_ANNOTATION`, and I liked `annot_attr_openmp` to match `annot_pragma_openmp`, but I don't feel terribly strongly.  See our conversation on TokenKinds for the other half of this discussion.
> > FWIW, C23 is getting C++ style attributes as well, so we MIGHT be able to make that assumption some day :D
> 
> And FWIW, I'm explicitly supporting OpenMP 5.1 attributes when -fdouble-square-bracket-attributes is enabled along with OpenMP 5.1 (which includes C23 mode by default). I just noticed that the OpenMP spec doesn't require this support in C, so should I remove that support in this patch (we can enable it as an extension when we allow it in older OpenMP modes), should I diagnose this as an extension only in C mode, or should I enable this as an extension in all OpenMP modes and add diagnostics for it?
> 
> > That said, the fact that these are called "PRAGMA_ANNOTATION" in TokenKinds.def seems misnamed to me anymore, which I think is the confusion. It is a little strange that the annot is added automatically, but the pragma isn't...
> 
> The fact that at least two people have found the name and definition of the token confusing means I'll be changing it. I think `ANNOTATION(attr_openmp)` will actually work out fine. The only use of the `PRAGMA_ANNOTATION` macro is in the definition of `tok::isPragmaAnnotation()` and the only places we call that function are places we're already looking for `tok::annot_pragma_openmp_from_attr` explicitly prior to the call anyway. The one oddity to this is that it starts with an `annot_attr_openmp` token and ends with an `annot_pragma_openmp_end` token -- but I don't think that should cause massive confusion (the end token is only interesting to the OpenMP parsing methods and those are designed around pragma terminology anyway).
> And FWIW, I'm explicitly supporting OpenMP 5.1 attributes when -fdouble-square-bracket-attributes is enabled along with OpenMP 5.1 (which includes C23 mode by default). I just noticed that the OpenMP spec doesn't require this support in C, so should I remove that support in this patch (we can enable it as an extension when we allow it in older OpenMP modes), should I diagnose this as an extension only in C mode, or should I enable this as an extension in all OpenMP modes and add diagnostics for it?

I would support all this stuff as an extension and emit a warning/note for the old versions, possibly ignored by default.


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

https://reviews.llvm.org/D105648