lebedev.ri added inline comments.
================
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:
> 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.
>
>> Here, it's is actually reasonably simple:
> Unfortunately, an 8-step process is anything but simple.
Hah, what i was saying is that it is entirely automatable,
all steps are identical each time.
> That's pretty much a definition of a "fragile test". The tests should check
> what they intend to check.
> I don't think LLVM does this, the CHECK lines are for the most part manually
> crafted.
No.
I //know// that 'most' of the new/updated tests are not manual,
from looking at the tests in commits, it's not an empty statement.
```
llvm/test/CodeGen$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll
| xargs grep "update_llc" | wc -l; done | sort -r -n -k 2,2
X86 1427
PowerPC 125
RISCV 90
...
```
```
llvm/test/CodeGen$ find X86/ -iname *.ll | wc -l
3370
llvm/test/CodeGen$ find PowerPC/ -iname *.ll | wc -l
1019
llvm/test/CodeGen$ find RISCV/ -iname *.ll | wc -l
105
```
So a half of X86 backend tests, and pretty much all RISCV backend tests.
```
llvm/test$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | xargs
grep "update_test" | wc -l; done | sort -r -n -k 2,2
Transforms 845
Analysis 17
CodeGen 6
Other 2
```
```
$ for i in `ls -1d *`; do echo -n "$i "; find $i -iname \*.ll | wc -l; done |
sort -r -n -k 2,2
Transforms 4620
Analysis 804
```
So sure, by the numbers, 'most' aren't auto-generated.
It's a question of legacy mainly.
I've both added llvm tests, and clang tests.
llvm tests are a breeze, just come up with sufficient IR,
and run the script [verify that the CHECK is what you thought it is,
or tune IR until it is], and you're done. With clang (codegen mostly),
one needs to first come up with the test, and then with the check lines,
and be really careful not to misspell `CHECK`, or else you're suddenly
not testing anything. clang tests are at least 3x more time-costly,
and i suspect thus have much less coverage they could have.
> If we need tests for AST dumper, we should write tests for it. OpenMP tests
> should not double for AST dumper tests.
I'll take a look at `StmtPrinterTest`, but i'm not sure i agree it's better.
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
