[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:269
+  /// or if this is an OpenMP stand-alone directive returns `None`.
+  llvm::Optional<Stmt *> getStructuredBlock() const;
 };
----------------
lebedev.ri wrote:
> 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.
> 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?

When deciding whether to check for null, we look at the function contract and check those pointers that are nullable.  For example, `IfStmt::getThen()` does not return NULL in valid ASTs, but `IfStmt::getElse()` does.

> 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.

Yes, I understand what optional is and how a new codebase could use it with pointers.  However, LLVM and Clang code has not been using optional to indicate nullability for pointers.  I could only find 136 occurrences of `Optional<.*\*>` in llvm-project.git.


================
Comment at: include/clang/Basic/OpenMPKinds.def:18
+#ifndef OPENMP_EXECUTABLE_DIRECTIVE
+#  define OPENMP_EXECUTABLE_DIRECTIVE(Name, Class)
+#endif
----------------
lebedev.ri wrote:
> 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.
It is used in Clang, for example, in `llvm/tools/clang/include/clang/AST/TypeNodes.def`, or `llvm/tools/clang/include/clang/AST/BuiltinTypes.def`.  I was surprised that this file does not use this pattern, to be honest.


================
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
+
----------------
lebedev.ri wrote:
> 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`)
> Here, it's is actually reasonably simple:

Unfortunately, an 8-step process is anything but simple.

> 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's pretty much a definition of a "fragile test".  The tests should check what they intend to check.

> That has been a very successful approach for LLVM.

I don't think LLVM does this, the CHECK lines are for the most part manually crafted.

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

If we need tests for AST dumper, we should write tests for it.  OpenMP tests should not double for AST dumper tests.



Repository:
  rC Clang

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

https://reviews.llvm.org/D59214