aaron.ballman added a comment. In D100762#2699350 <https://reviews.llvm.org/D100762#2699350>, @dexonsmith wrote:
> In D100762#2699252 <https://reviews.llvm.org/D100762#2699252>, @arichardson > wrote: > >> I'm not sure it's a good idea to remove the `-ast-dump=json` option. While >> this is -cc1 option, there do seem to be external consumers based on a quick >> search for "-ast-dump=json". Keeping it would also reduce some of the test >> churn. > > Maybe `-ast-dump=json` can be changed to an alias for `-ast-dump > -ast-dump-format json`. +1 to this idea ================ Comment at: clang/test/AST/ast-dump-comment-json.cpp:44 // CHECK-NEXT: "loc": { -// CHECK-NEXT: "offset": 72, +// CHECK-NEXT: "offset": 89, // CHECK-NEXT: "line": 3, ---------------- dexonsmith wrote: > This is a lot of noise in the tests just from changing `RUN` lines. Maybe > these tests shouldn't be checking the `offset:` field. > > I suggest: > 1. Create (if it doesn't exist) one **small** test that checks that `offset:` > works correctly. In the same commit, replace the `offset:` lines in all the > other tests to check against `[[0-9+]],` instead of the specific character > offset. > 3. Land this change, which no longer needs to update all the `offset:` fields. Offsets are pretty important to ensure they're sensible, but we probably don't need to check offsets in all tests, so I think this suggestion makes sense. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100762/new/ https://reviews.llvm.org/D100762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
