Page MenuHomePhabricator

[clang][cli] Extract AST dump format into extra option
Changes PlannedPublic

Authored by jansvoboda11 on Mon, Apr 19, 7:13 AM.

Details

Summary

When dumping AST, the JSON format can be requested in two ways: -ast-dump=json and -ast-dump-all=json. Both of those command line options control FrontendOptions::ASTDumpFormat.

This patch deduplicates the options by introducing -ast-dump-format, removes the _EQ variants of -ast-dump and -ast-dump-all, and simplifies option parsing/generation.

Diff Detail

Event Timeline

jansvoboda11 created this revision.Mon, Apr 19, 7:13 AM
jansvoboda11 requested review of this revision.Mon, Apr 19, 7:13 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptMon, Apr 19, 7:13 AM
dexonsmith requested changes to this revision.Mon, Apr 19, 11:32 AM
dexonsmith added inline comments.
clang/test/AST/ast-dump-comment-json.cpp
44

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.
  2. Land this change, which no longer needs to update all the offset: fields.
This revision now requires changes to proceed.Mon, Apr 19, 11:32 AM
arichardson requested changes to this revision.Mon, Apr 19, 11:43 AM
arichardson added a reviewer: aaron.ballman.

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.

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.

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

clang/test/AST/ast-dump-comment-json.cpp
44

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.

jansvoboda11 planned changes to this revision.Thu, Apr 22, 2:23 AM

If -ast-dump=json was a driver flag, it would be trivial to pass -ast-dump -ast-dump-format json to -cc1 instead. However, aliasing a single option to two options within the -cc1 argument parser isn't possible at the moment AFAIK. I can look into how much work adding that capability would be.

@arichardson Can you point me to the external consumers?

@dexonsmith I agree checking the offset only in a couple of tests would be better in terms of churn.

If -ast-dump=json was a driver flag, it would be trivial to pass -ast-dump -ast-dump-format json to -cc1 instead. However, aliasing a single option to two options within the -cc1 argument parser isn't possible at the moment AFAIK. I can look into how much work adding that capability would be.

@arichardson Can you point me to the external consumers?

I just did the following search and saw that there are multiple stack overflow answers etc. recommending the use of -Xclang -ast-dump=json: https://www.google.com/search?q=%22-ast-dump%3Djson%22

I am not sure how many actual consumers there are, but I think it would be good to keep this option to avoid surprises for users. While they probably shouldn't be using internal -cc1 options, this is the only documented way of getting an AST dump.

If -ast-dump=json was a driver flag, it would be trivial to pass -ast-dump -ast-dump-format json to -cc1 instead. However, aliasing a single option to two options within the -cc1 argument parser isn't possible at the moment AFAIK. I can look into how much work adding that capability would be.

@arichardson Can you point me to the external consumers?

I just did the following search and saw that there are multiple stack overflow answers etc. recommending the use of -Xclang -ast-dump=json: https://www.google.com/search?q=%22-ast-dump%3Djson%22

I am not sure how many actual consumers there are, but I think it would be good to keep this option to avoid surprises for users. While they probably shouldn't be using internal -cc1 options, this is the only documented way of getting an AST dump.

This gets used on Compiler Explorer as it's the only way to get a JSON dump from that interface. e.g., https://godbolt.org/z/3nf6fvfYz