[PATCH] D71709: Give frontend dump flags consistent names (*-dump instead of dump-*)

Brian Gesiak via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Dec 22 15:30:46 PST 2019


modocache added a subscriber: mehdi_amini.
modocache added a comment.

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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71709/new/

https://reviews.llvm.org/D71709





More information about the cfe-commits mailing list