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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
