[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/StmtOpenMP.h:266
+
+  /// Returns the AST statement representing OpenMP structured-block of this
+  /// OpenMP executable directive,
----------------
"the AST node"


================
Comment at: include/clang/AST/StmtOpenMP.h:2158
 
+  /// OpenMP flush construct is a stand-alone directive.
+  llvm::Optional<Stmt *> getStructuredBlockImpl() const { return llvm::None; };
----------------
This comment (and other similar comments on getStructuredBlockImpl()) are implementation comments and should be written in the function body.


================
Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif
----------------
Why not add a default definition:

```
#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
```

(Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)


================
Comment at: test/AST/ast-dump-openmp-atomic.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fopenmp -ast-dump %s | FileCheck -strict-whitespace -implicit-check-not=openmp_structured_block %s
+
----------------
I'm not a fan of ast-dump tests.  They are fragile, difficult to update, difficult to read (when they are 700 lines long), and it is unclear what the important parts are.

WDYT about converting them to unit tests?  See `clang/unittests/AST/StmtPrinterTest.cpp` for an example.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214