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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr 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().
----------------
"non-standalone OpenMP executable directives"


================
Comment at: test/PCH/stmt-openmp_structured_block-bit.cpp:8
+
+// expected-no-diagnostics
+
----------------
Maybe try imitating a more recently written test, like `llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?  It has both header and user written in the same file, for improved readability.


================
Comment at: test/PCH/stmt-openmp_structured_block-bit.h:2
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/cxx_exprs.h -std=c++11 -fsyntax-only -verify %s -ast-dump | FileCheck %s
+
----------------
Is cxx_exprs.h needed?

I don't think `-std=c++11` is needed either.

Same in RUN lines below.

Maybe try imitating a more recently written test, like `llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?


================
Comment at: test/PCH/stmt-openmp_structured_block-bit.h:5
+// Test with pch. Use '-ast-dump' to force deserialization of function bodies.
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s -ast-dump-all | FileCheck %s
----------------
The point of PCH tests is to test the serialization of ASTs into PCH.  In this line the cxx_exprs.h header is being initialized, it does not contain any OpenMP constructs, so I'm not sure what this test is supposed to test.


================
Comment at: test/PCH/stmt-openmp_structured_block-bit.h:6
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s -ast-dump-all | FileCheck %s
+
----------------
I don't see any CHECK lines for FileCheck.  (FileCheck fails the test if there are no CHECK lines.)


================
Comment at: unittests/AST/OMPStructuredBlockTest.cpp:9
+//
+// Fine-grained tests for IsOMPStructuredBlock bit of Stmt's.
+//
----------------
"of Stmt"


================
Comment at: unittests/AST/OMPStructuredBlockTest.cpp:27
+
+AST_MATCHER(Stmt, isOMPStructuredBlock) { return Node.isOMPStructuredBlock(); }
+
----------------
Eventually you'll move it to `ASTMatchers.h`, right?


================
Comment at: unittests/AST/OMPStructuredBlockTest.cpp:41
+
+StatementMatcher OMPStandaloneDirectivekMatcher() {
+  return stmt(ompExecutableDirective(isStandaloneDirective())).bind("id");
----------------
extra "k" before "Matcher"


================
Comment at: unittests/AST/PrintTest.h:15
+
+using namespace clang;
+using namespace ast_matchers;
----------------
Please don't write `using namespace` in headers.  Instead, put the code below into the `clang` namespace.


================
Comment at: unittests/AST/PrintTest.h:19
+
+namespace {
+
----------------
No need for this, make `PrintStmt()` static.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214