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: > 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. >> 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. Blargh. I meant to write `"structured block of *standalone* executable directives" would make no sense`. > ".." Yes, that looks cleaner, thank you! ================ Comment at: unittests/AST/OMPStructuredBlockTest.cpp:64 +++i; +})"; + ASSERT_TRUE( ---------------- gribozavr wrote: > 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; > })"; > ``` > > 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? Absolutely. 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
