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