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