[clang] [clang][NFC] Reduce one of JSON dump tests (PR #195026)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 30 08:25:40 PDT 2026
AaronBallman wrote:
> All of these commits have changes to the JSON dump part of this file which are relevant to the commit in question: [fb02433](https://github.com/llvm/llvm-project/commit/fb024337497f4c158ea16bfb90e50138d9a2a0c6) [91ebd4a](https://github.com/llvm/llvm-project/commit/91ebd4ac4ac4b11653a26b3a04af5b94ccda7e4c) [696946f](https://github.com/llvm/llvm-project/commit/696946f897db90d07dbfb981ad8e75fd872a1d35) [398f6c2](https://github.com/llvm/llvm-project/commit/398f6c271d4b408f7e783c29d5bfcd5413eae76d) [ba9d1c4](https://github.com/llvm/llvm-project/commit/ba9d1c41c41d568a798e0a8c38a89d294647c28d) [0e33195](https://github.com/llvm/llvm-project/commit/0e3319537972d284e94a183899f15b40a798fd1f) [eeada0d](https://github.com/llvm/llvm-project/commit/eeada0d30f41b2d680073e49246ac0388e6c5ec7) [91cdd35](https://github.com/llvm/llvm-project/commit/91cdd35008e9ab32dffb7e401cdd7313b3461892) [13b55ad](https://github.com/llvm/llvm-project/commit/13b55ad3bb6bafda7d8a62c5fe2fc98157442355) [dc17429](https://github.com/llvm/llvm-project/commit/dc17429ae6961a6783371dcf6749eea657b5446a) [89cfeeb](https://github.com/llvm/llvm-project/commit/89cfeeb062577069d1da236d33810bb0416f1102) [14f7bd6](https://github.com/llvm/llvm-project/commit/14f7bd63b95d0f61a6f47119ac66398ca230559a)
I'm not going to spend much more time on review here because it's getting to be extractive at this point, but some of these commits have me worried that AST dumping tests are being used instead of more appropriate testing mechanisms. For example, commit eeada0d30f41b2d680073e49246ac0388e6c5ec7 should not be using AST dumping tests, it can use diagnostics to test the source range information. We have `-fdiagnostics-print-source-range-info` specifically to help test source ranges. Commit 89cfeeb062577069d1da236d33810bb0416f1102 added only AST dumping tests despite changing structural equivalence code that could have been tested through other means like serialization related diagnostics. Commit 91cdd35008e9ab32dffb7e401cdd7313b3461892 changed 539 files and is not really an example of the kinds of PRs we want generally; it's not easy to tell how the AST dumping changes relate to the PR or its changes.
tl;dr: as lead maintainer, (in general) I want *less* AST dumping tests to be added in tree, for JSON or otherwise. They have become too much of a maintenance burden for something which is intended to be used as a debugging and research aid. Having regression tests to ensure we don't crash is a reasonable thing for us to test, but to my mind, they are not to be used for exhaustive testing unless there's a strong justification for why the testing shouldn't use less fragile tools. If someone wants to invest effort in the scripts to make them more usable, I think that's worthwhile, but as the scripts are today, I'd be fine removing them entirely from the tree so others don't mistakenly use them.
https://github.com/llvm/llvm-project/pull/195026
More information about the cfe-commits
mailing list