aaron.ballman marked 8 inline comments as done. aaron.ballman 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); + ---------------- ABataev wrote: > 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. Okay, I can do that. 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
