[PATCH] D100762: [clang][cli] Extract AST dump format into extra option

Wed Apr 21 06:46:41 PDT 2021

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