Give frontend dump flags consistent names (*-dump instead of dump-*)

[herzka]

Summary

These now match other flags like -ast-dump*

[herzka]

@modocache, should I add a unit test for these aliases or would that be too trivial? I don't see unit tests for all the existing flags.

[herzka]

Update argument parsing

[modocache]

Grepping for "dump-tokens", I can see one regression test that exercises this option: clang/test/Lexer/dollar-idents.c. "dump-coverage-mapping" also shows up in a few regression tests. One way to exercise the new option names is to modify these tests to use both the new and old option names. I think this would test that they can be used interchangeably.

As for the unit tests in llvm/unittests or clang/unittests, I agree, I don't think this diff requires any new tests in there.

I'll request changes on this diff because some switch statements in Clang need to be updated to handle the new option names. Also I'll suggest some additional reviewers: @jroelofs (who suggested we create aliases for these), and @echristo (because these options were last modified over 7 years ago and Eric has a long tenure in this codebase).

[herzka]

I worry that repeating all those names in those tests might be noisy. I think I'll switch those to use the new name, but add another test that makes sure both names still work.

[herzka]

change flags in tests

[modocache]

Ah, I'm sorry I wasn't clear -- instead of changing a lot of tests to use the new names exclusively, my suggestion was to change one or two tests to use the new canonical name, and have the remaining tests keep using the alias. That would keep the patch small, but still exercise both the new option name -tokens-dump as well as the old alias -dump-tokens.

That being said, this change also works for me! I'd be happy to have this landed. But I want to make sure others have input as well. @mehdi_amini commented on this patch on the LLVM Discord, for example:

This patch seems weird to me: with this logic we would shuffle around every possible options (-fdump-vtable-layouts / -fvtable-layouts-dump, -disable-O0-optnone / -O0-optnone-disable, ...) ?

To which I responded:

I'm of the opinion -- and maybe this would be a radical change -- that all "dump" options should either use it as a prefix (-fdump-vtable-layouts) or as a suffix (-fvtable-layouts-dump). I'm not an expert though, so I don't know what would be the polite way to change option names without disrupting end users of LLVM. I thought the patch was good because it adds aliases, so people who are used to -dump-tokens or -ast-dump could still use those option names.

So I think my answer to your question is "no, we shouldn't provide aliases with all permutations of words in options." My take is "for options that dump some sort of intermediate output, dump should appear in a standard place in the option name. Whether it's at the beginning (-dump-ast, -dump-tokens) or at the end (-ast-dump, -tokens-dump) doesn't matter to me, per se, as long as it's consistent." And, my second opinion: "Personally I'd love to change all of these option names right now, but that might be disruptive, I don't know. If it isn't, then let's change them. If it is, then using aliases seems like a good migratory plan."

I'd like to wait and give others the chance to review this patch, so I'll wait a bit to accept it. It would help me personally day to day to have this change landed, because I can never remember the order of options like -tokens-dump and -dump-ast.

[herzka]

rebase, restore changes to CC1Options.td that I left out of last revision

clang/include/clang/Driver/CC1Options.td

 def coverage_no_function_names_in_data : Flag<["-"], "coverage-no-function-names-in-data">,
   HelpText<"Emit function names in .gcda files.">;
 def coverage_exit_block_before_body : Flag<["-"], "coverage-exit-block-before-body">,
   HelpText<"Emit the exit block before the body blocks in .gcno files.">;
 def coverage_version_EQ : Joined<["-"], "coverage-version=">,
   HelpText<"Four-byte version string for gcov files.">;
 def test_coverage : Flag<["-"], "test-coverage">,
   HelpText<"Do not generate coverage files or remove coverage changes from IR">;
-def dump_coverage_mapping : Flag<["-"], "dump-coverage-mapping">,
+def coverage_mapping_dump : Flag<["-"], "coverage-mapping-dump">,
   HelpText<"Dump the coverage mapping records, for testing">;
+def dump_coverage_mapping : Flag<["-"], "dump-coverage-mapping">,
+  Alias<coverage_mapping_dump>;
 def fuse_register_sized_bitfield_access: Flag<["-"], "fuse-register-sized-bitfield-access">,
   HelpText<"Use register sized accesses to bit-fields, when possible.">;
 def relaxed_aliasing : Flag<["-"], "relaxed-aliasing">,
   HelpText<"Turn off Type Based Alias Analysis">;
 def no_struct_path_tbaa : Flag<["-"], "no-struct-path-tbaa">,
   HelpText<"Turn off struct-path aware Type Based Alias Analysis">;
 def new_struct_path_tbaa : Flag<["-"], "new-struct-path-tbaa">,
   HelpText<"Enable enhanced struct-path aware Type Based Alias Analysis">;
HelpText<"introduce a module file extension for testing purposes. "
            "The argument is parsed as blockname:major:minor:hashed:user info">;
 def fconcepts_ts : Flag<["-"], "fconcepts-ts">,
   HelpText<"Enable C++ Extensions for Concepts.">;
 
 let Group = Action_Group in {
 
 def Eonly : Flag<["-"], "Eonly">,
   HelpText<"Just run preprocessor, no output (for timings)">;
-def dump_raw_tokens : Flag<["-"], "dump-raw-tokens">,
+def raw_tokens_dump : Flag<["-"], "raw-tokens-dump">,
   HelpText<"Lex file in raw mode and dump raw tokens">;
+def dump_raw_tokens : Flag<["-"], "dump-raw-tokens">,
+  Alias<raw_tokens_dump>;
 def analyze : Flag<["-"], "analyze">,
   HelpText<"Run static analysis engine">;
-def dump_tokens : Flag<["-"], "dump-tokens">,
+def tokens_dump : Flag<["-"], "tokens-dump">,
   HelpText<"Run preprocessor, dump internal rep of tokens">;
+def dump_tokens : Flag<["-"], "dump-tokens">,
+  Alias<tokens_dump>;
 def init_only : Flag<["-"], "init-only">,
   HelpText<"Only execute frontend initialization">;
 def fixit : Flag<["-"], "fixit">,
   HelpText<"Apply fix-it advice to the input source">;
 def fixit_EQ : Joined<["-"], "fixit=">,
   HelpText<"Apply fix-it advice creating a file with the given suffix">;
 def print_preamble : Flag<["-"], "print-preamble">,
   HelpText<"Print the \"preamble\" of a file, which is a candidate for implicit"

 def pic_level : Separate<["-"], "pic-level">,
   HelpText<"Value for __PIC__">;
 def pic_is_pie : Flag<["-"], "pic-is-pie">,
   HelpText<"File is for a position independent executable">;
 def fno_validate_pch : Flag<["-"], "fno-validate-pch">,
   HelpText<"Disable validation of precompiled headers">;
 def fallow_pch_with_errors : Flag<["-"], "fallow-pch-with-compiler-errors">,
   HelpText<"Accept a PCH file that was created with compiler errors">;
-def dump_deserialized_pch_decls : Flag<["-"], "dump-deserialized-decls">,
+def deserialized_pch_decls_dump : Flag<["-"], "deserialized-decls-dump">,
   HelpText<"Dump declarations that are deserialized from PCH, for testing">;
+def dump_deserialized_pch_decls : Flag<["-"], "dump-deserialized-decls">,
+  Alias<deserialized_pch_decls_dump>;
 def error_on_deserialized_pch_decl : Separate<["-"], "error-on-deserialized-decl">,
   HelpText<"Emit error if a specific declaration is deserialized from PCH, for testing">;
 def error_on_deserialized_pch_decl_EQ : Joined<["-"], "error-on-deserialized-decl=">,
   Alias<error_on_deserialized_pch_decl>;
 def static_define : Flag<["-"], "static-define">,
   HelpText<"Should __STATIC__ be defined">;
 def stack_protector : Separate<["-"], "stack-protector">,
   HelpText<"Enable stack protectors">;