[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 08:49:06 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:
> 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





More information about the cfe-commits mailing list