[PATCH] D99905: [OPENMP51]Initial parsing/sema for adjust_args clause for 'declare variant'

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some nits.



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1447-1448
+    if (IsError) {
+      while (!SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch))
+        ;
+      // Skip the last annot_pragma_openmp_end.
----------------
ABataev wrote:
> aaron.ballman wrote:
> > ABataev wrote:
> > > aaron.ballman wrote:
> > > > ABataev wrote:
> > > > > aaron.ballman wrote:
> > > > > > I know this is code that existed previously, but why the `while` loop? This strikes me as a potential infinite loop because `SkipUntil` returns `false` when the token is not found, so if the token is not found the first time, I'd be surprised if a second pass at it finds the token.
> > > > > It may return early because of parens/braces etc. counting. In this case, it will return `false` while if `annot_pragma_openmp_end` is found, it returns `true`. We need to iterate to the end of the directive here, that's why there is a loop.
> > > > Thank you for the explanation -- that sure sounds like a bug with `SkipUntil` to me, and looking through the code base, the only code that seems to do this dance is OpenMP code. Do you happen to have a code example that requires the loop behavior?
> > > > 
> > > > (Not suggesting a change needs to be made in this patch, I'm mostly wondering if there's a change we can make in a follow-up and remove these loops.)
> > > OpenMP implementation guarantees that the directives are started by `annot_pragma_openmp_begin` and terminated by `annot_pragma_openmp_end`. That's why we always trying to find the terminating `annot_pragma_openmp_end` to recover the parsing errors in OpenMP correctly, so the parser could start parsing regular C/C++ code rather than starting parsing in the middle of the OpenMP directive.
> > > There are some tests in test/OpenMP directory that checks for this but I don't remember which one exactly.
> > > I think `SkipUntil` works correctly for the regular C/C++ code, it is just a special corner case for the pragmas 
> > > OpenMP implementation guarantees that the directives are started by annot_pragma_openmp_begin and terminated by annot_pragma_openmp_end. That's why we always trying to find the terminating annot_pragma_openmp_end to recover the parsing errors in OpenMP correctly, so the parser could start parsing regular C/C++ code rather than starting parsing in the middle of the OpenMP directive.
> > 
> > My expectation is that calling `SkipUntil(annot_pragma_openmp_end)` should skip until it hits the given annotation token, the end of directive marker, or the end of file marker. It should not require a `while` loop to reach the given token properly. Either the call returns `true` because it found the token, or it returns `false` and it advanced the token to some reasonable stopping point that I would expect is outside of the stream of directive tokens.
> > 
> > Given that there is a guarantee that the directive ends with a `annot_pragma_openmp_end`, I don't see why a loop should be necessary for callers (one may be necessary within `SkipUntil`, obviously).
> We need to disable braces/parens counting in `SkipUntil` for some cases to avoid loops.
That continues to sound like a bug with `SkipUntil()` to me, but one that's orthogonal to this patch.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:4161
+        Data.ColonLoc =  Tok.getLocation();
+      ExpectAndConsume(tok::colon, diag::warn_pragma_expected_colon, "adjust-op");
+    }
----------------
You should fix the formatting issue.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:7238
+    Diag(E->getExprLoc(), diag::err_omp_param_or_this_in_clause)
+        << FD->getDeclName() << 0;
+    return;
----------------



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

https://reviews.llvm.org/D99905