[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D158137#4599546 <https://reviews.llvm.org/D158137#4599546>, @dblaikie wrote:

> In D158137#4599481 <https://reviews.llvm.org/D158137#4599481>, @MaskRay wrote:
>
>> In D158137#4599461 <https://reviews.llvm.org/D158137#4599461>, @dblaikie wrote:
>>
>>> In D158137#4597491 <https://reviews.llvm.org/D158137#4597491>, @dexonsmith wrote:
>>>
>>>> In D158137#4597009 <https://reviews.llvm.org/D158137#4597009>, @MaskRay wrote:
>>>>
>>>>> In D158137#4596948 <https://reviews.llvm.org/D158137#4596948>, @dexonsmith wrote:
>>>>>
>>>>>> Can you explain the downside of leaving behind an alias?
>>>>>
>>>>> Two minor ones. (a) Existing `-Wno-overriding-t-option` will not notice that they need to migrate and (b) Clang has accrued tiny tech debt.
>>>>> If we eventually remove `-Wno-overriding-t-option` for tidiness, we will have to break `-Werror -Wno-overriding-t-option` users.
>>>>
>>>> I guess it's not clear to me we'd need to remove the alias. The usual policy (I think?) is that clang driver options don't disappear. It seems like a small piece of debt to maintain the extra alias in this case, and if it's kept, then users don't actually need to migrate. And then you can feel safe updating Darwin.cpp as well.
>>>
>>> +1 to this, FWIW - I wouldn't consider it technical debt to keep a compatible warning flag name that's been around for a decade & isn't a name we're trying to free up for some other use or because it causes any great confusion, etc.
>>
>> I think my previous comment has answered this.
>> Think: every Clang release may emit some new warnings. `-Werror` users has actually provided a promise that they will fix reasonable toolchain changes. Toolchain contributors check how disruptive a change will be, but cannot promise that we never emit new warnings.
>> In this case, `overriding-t-options` seems a fairly rare and LLVM/Clang side has far too many other fp-model/fast-math changes to make "whether we will get a new warning" a fairly minor issue.
>> I am unfamiliar with Darwin.cpp though. If it turns out that want to disable the warning even with `-Wno-overriding-t-option`, we can add a workaround specific to Darwin.cpp, but not to fp-model/Tc.
>
> Sure, I understand that we can break these things - like if we totally remove a warning, I wouldn't be averse to removing the flag/not leaving it there for backwards "compatibility" (when it's misleading/doesn't actually trigger the warning, for instance). But in this case it seems like we're keeping the functionality, decided on a better name, but it seems pretty harmless and somewhat beneficial to keep the old name around.

IMO, the less friction we make for users doing an upgrade, the better. I tend to agree with @dblaikie that it might be reasonable to leave the old flag around as an alias for the new flag (we can document a preference for the new name if it makes us feel better).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158137