[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

Roman Lebedev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 20 07:39:28 PDT 2019


lebedev.ri added inline comments.


================
Comment at: include/clang/AST/Stmt.h:102
+    /// This bit is set only for the Stmt's that are the structured-block
+    /// of OpeMP [non-standalone] executable directives.
+    /// I.e. those returned by OMPExecutableDirective::getStructuredBlock().
----------------
gribozavr wrote:
> "non-standalone OpenMP executable directives"
English isn't my native language; to me the current variant (attempts to?) convey
that structured block can only ever exist for non-standalone executable directives.
I.e. "structured block of non-standalone executable directives" is a misnomer.

The variant without `[]` does not convey any of that, i think, it just says
that "it's a structured block of non-standalone OpenMP executable directives",
it isn't obvious from that (without knowing OpenMP details) that there 
wouldn't be "a structured block of standalone OpenMP executable directive".

Isn't there an important difference there?


================
Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64
+++i;
+})";
+  ASSERT_TRUE(
----------------
gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > Same comment as in the other patch -- I would prefer that the source is inlined into the ASSERT_TRUE, with implicit string concatenation and "\n"s, if raw strings don't work.
> > I still don't understand the reasoning here.
> > It feels like a change just for the sake of the change.
> These variables don't improve readability.  Also, if there are multiple such variables in one test, it is easy to mistakenly use an incorrect one in the ASSERT_TRUE line.
Can you please quote something about this from
https://llvm.org/docs/CodingStandards.html
https://llvm.org/docs/ProgrammersManual.html
etc?
If not this is entirely subjective/stylistic,
and this was the form that was more readable to me.
(easier to write, too, those "\n" will look horrible)
Also, what about the cases with two assertions? (not just the new ones)

I really don't see how that would be a good change.


================
Comment at: unittests/AST/OMPStructuredBlockTest.cpp:656
+
+TEST(OMPStructuredBlock, DISABLED_TestParallelMaster0XFAIL) {
+  const char *Source =
----------------
gribozavr wrote:
> Is it only a pretty-printer problem or something more severe?
[[ https://bugs.llvm.org/show_bug.cgi?id=41022 | PR41022 ]].


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214





More information about the cfe-commits mailing list