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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added subscribers: steveire, aaron.ballman.
lebedev.ri added a comment.

Thanks for taking a look, some replies.



================
Comment at: include/clang/AST/StmtOpenMP.h:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional<Stmt *> getStructuredBlock() const;
 };
----------------
gribozavr wrote:
> Why not `Stmt *` as a return value?
Good question.

Because usually checking a pointer for null is an exceptional
case, you //normally// have the AST node, you don't check
**every single** pointer for `null`, i don't think?

This is absolutely not the case here,  `null` is **not** an
exceptional failure state here, it's the expected return value
for stand-alone directives. With `Optional`, you explicitly
signal "hey, i'm not a plain pointer, pay attention!",
thus hopefully maybe preventing some issues.

I'm not sure we can more nicely solve this with an extra base class[es],
at least not without having only two direct subclasses of `OMPExecutableDirective`:
* `OMPNonstandaloneExecutableDirective`.
* `OMPStandaloneExecutableDirective`.
and two different `OMPOrderedDirective` classes (one standalone, one not).
But somehow i don't think there is any chance of that being accepted,
even though it would result in slightly cleaner AST.


================
Comment at: include/clang/AST/StmtOpenMP.h:335
 
+  llvm::Optional<Stmt *> getStructuredBlockImpl() const {
+    return const_cast<Stmt *>(getInnermostCapturedStmt()->getCapturedStmt());
----------------
ABataev wrote:
> No need to insert it into each class, just add:
> ```
> Stmt * OMPExecutableDirective::getStructuredBlock() const {
>   if (!hasAssociatedStmt() || !getAssociatedStmt())
>     return nullptr;
>   if (auto *LD = dyn_cast<OMPLoopDirective>(this))
>     return LD->getBody();
>   return getInnermostCapturedStmt()->getCapturedStmt();
> }
> ```
I absolutely can do that, you are sure that is the most future-proof state?
In particular, i want to re-point-out that if it's implemented like this,
in the base class, then the sub-class may(will) not even know about this function,
and thus 'forget' to update it, should it not be giving the correct answer for
that new specific OpenMP executable directive.

You are sure it's better to implement it in the `OMPExecutableDirective` itself?


================
Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif
----------------
gribozavr wrote:
> Why not add a default definition:
> 
> ```
> #  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class) OPENMP_DIRECTIVE(Name)
> ```
> 
> (Also for `OPENMP_EXECUTABLE_DIRECTIVE_EXT` below.)
Hm, i have never seen that chaining pattern in `.def` headers before,
so it could be surprising behavior.


================
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
+
----------------
gribozavr wrote:
> 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.
> They are difficult to update.

The updating part is true, for all of the clang tests,
they unlike llvm tests have no update scripts, so it's a major pain.
Here, it's is actually reasonably simple:
1. prune old `// CHECK*` lines
2. run the run line
3. prepend each line of output with `// CHECK-NEXT: `
4. scrub addresses, `s/0x[0-9a-f]+/0x{{.*}}/`
5. scrub filepath prefix, again a simple string-replace
6. prune everything after `TranslationUnitDecl` and before first `FunctionDecl`
7. replace a few `// CHECK-NEXT: ` with `// CHECK: `
8. append the final processed output to the test file. done.

It would not be too hard to write an update tool for this, but i've managed with a simple macro..

> They are fragile, difficult to read (when they are 700 lines long),
> and it is unclear what the important parts are.

This is kind-of intentional. I've tried to follow the llvm-proper approach of gigantic 
tests that are overly verbose, but quickly cement the state so **any** change **will**
be noticed. That has been a **very** successful approach for LLVM.

In fact, the lack of this ast-dumper test coverage was not helping
the recent ast-dumper refactoring, ask @aaron.ballman / @steveire.

So no, i really don't want to //convert// the tests into a less verbose state.

I suppose i //could// //add// a more fine-grained tests, 
though readability is arguable. Here, it is obvious nothing is omitted,
and you can quickly see the `openmp_structured_block` by Ctrl+F'ing for it.
With more distilled tests, it's less obvious. (i'm not talking about `StmtPrinterTest`)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214