dexonsmith added inline comments.
================ Comment at: clang/test/ExtractAPI/enum.c:11 +// RUN: %t/output.json >> %t/output-normalized.json +// RUN: diff %t/reference.output.json %t/output-normalized.json + ---------------- Diffing against reference output is usually not ideal: - It makes it hard to evolve the output, since many unrelated tests could be affected and need an update. In practice, people may just regenerate the output without really knowing if the essential feature is still behaving correctly. - It makes it hard to evolve the tests, since line numbers are hardcoded. Is it possible to update to use `FileCheck` for the output? You can just check the parts of the output that are relevant for this specific test, embed `CHECK`s directly into the input file, and use `@LINE` so that the line numbers tied to the CHECK comments instead of hardcoded. ================ Comment at: clang/test/ExtractAPI/enum.c:13-14 + +// CHECK-NOT: error: +// CHECK-NOT: warning: + ---------------- dexonsmith wrote: > It's usually much easier to test diagnostics using clangs builtin `-verify` > flag instead of FileCheck. `-verify` is documented in the header: > https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h > (maybe elsewhere too?). > > This gives you good diffs between expected and unexpected diagnostics. In > this case, you could replace these `CHECK-NOT`s with: > ``` > // expected-no-diagnostics > ``` > Usually you'd put that after the `RUN` line. When diagnostics are expected, > you usually put those comments on the same line where you expect the > diagnostic (there are tricks to avoid long lines and to handle other > locations; see the docs). As an example I just posted https://reviews.llvm.org/D124634 for this file; PTAL, and then it'd be great if you could update the other tests when you get a chance. ================ Comment at: clang/test/ExtractAPI/enum.c:136 + "character": 22, + "line": 1 + }, ---------------- (I noticed the `diff` when adding `// expected-no-diagnostics` as part of https://reviews.llvm.org/D124634 -- I couldn't add it to the top of the input file, since that disrupted all of the line numbers and caused the `diff` to fail. An easy workaround in this case was just to move it to the end of the file so I wasn't blocked but it'd be better if the test were able to evolve over time I think.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121873/new/ https://reviews.llvm.org/D121873 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
