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