gribozavr accepted this revision. gribozavr added inline comments. This revision is now accepted and ready to land.
================ 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(). ---------------- lebedev.ri wrote: > 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? I didn't catch this difference implied by "[]". > I.e. "structured block of non-standalone executable directives" is a misnomer. Why is it a misnomer? It might be overly verbose, but it is correct. However, I see that this fact is important to highlight, so I'd suggest to spell it out: "This bit is set only for the Stmts that are the structured-block of OpenMP executable directives. Directives that have a structured block are called "non-standalone" directives." Please also fix the typo "OpeMP" => OpenMP. ================ Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64 +++i; +})"; + ASSERT_TRUE( ---------------- lebedev.ri wrote: > 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. There are lots of readability points that are not formalized in the coding style, and yet, are followed in the code. For cases with two assertions we need to have a variable to avoid code duplication, so it is OK there -- the variable has a purpose. If you want to use raw string literals, could I at least ask you to drop the first line onto the same level of indentation as the rest of the lines? ``` const char *Source = R"(void test(int i) { #pragma omp atomic ++i; })"; ``` change to: ``` const char *Source = R"( void test(int i) { #pragma omp atomic ++i; })"; ``` 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
