warn_drv_overriding_flag_option was added for clang-cl /T* options (D1290) and its group name was planned to be renamed to overriding-option.
The name -Woverriding-t-option does not make sense for other uses,
mostly related to -ffp-model= related diagnostics.
Details
- Reviewers
aaron.ballman andrew.w.kaylor hans skan zahiraam dexonsmith - Group Reviewers
Restricted Project - Commits
- rG1c66d08b0137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM.
Perhaps as a follow-up, rename warn_drv_overriding_flag_option to have “t” in it?
Thanks! I agree. d9ad0681fad9a98f43d9baddb95d505b37153c48 (2013) renamed warn_drv_overriding_t_option to warn_drv_overriding_flag_option.
Perhaps the original name warn_drv_overriding_t_option should be restored.
Thanks! I agree. d9ad0681fad9a98f43d9baddb95d505b37153c48 (2013) renamed warn_drv_overriding_t_option to warn_drv_overriding_flag_option.
Perhaps the original name warn_drv_overriding_t_option should be restored.
That change also started using it for overriding /MD and /MT. I think the intention then was to make the flag more general, but it forgot to rename the flag spelling. Perhaps the fix is really to s/overriding-t-option/overriding-option/ ? (If we're concerned about the interface change, we could retain an alias with the old spelling.)
s/overriding-t-option/overriding-option/ sounds good. I'll change the patch accordingly, but keep the warn_drv_overriding_option name (Some uses are not related to boolean flags).
I wish that we keep -Woverriding-t-option and -Woverriding-option different.
This seems to drop -Woverriding-t-option entirely. Could that break builds if someone has (e.g.) -Werror -Wno-overriding-t-option in their build settings?
Yes. We could also add an ignored -Wno-overriding-t-option to Options.td. Users who want to ignore the existing diagnostics need to add -Wno-overriding-option now.
At some point we can add clang_ignored_legacy_options_Group to -Wno-overriding-option so that there will be a warning (error if -Werror)?
Even if we don't drop -Woverriding-t-option, as long as we make -Woverriding-t-option disconnected from the code we want to change (-ffp-model= related diagnostics), -Werror users will observe an error. It seems unavoidable for them to observe an error, unless we make overriding-t-option an alias for overriding-option (which we don't want to).
For non -Werror users, removing -Woverriding-t-option is not a problem. An unknown -W gives a warning.
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.
The difference is what error message we want to display:
// no change to Options.td warning: unknown warning option '-Wno-overriding-t-option'; did you mean '-Wno-overriding-option'? [-Wunknown-warning-option] // def : Flag<["-"], "Wno-overriding-t-option">, Flags<[Ignored]>; warning: argument unused during compilation: '-Wno-overriding-t-option' [-Wunused-command-line-argument]
My understanding of -Werror users is that they may have to fix reasonable toolchain updates. Specifying -Wno-overriding-t-option for -ffp-model= and D41425 is odd in the first place.
https://gitlab.kitware.com/cmake/cmake/-/issues/20132 is probably the reason I want to keep -Wno-overriding-t-option for Darwin.cpp, even if the option name is strange.
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.
-W* options are different from regular driver options in that -Wunknown-unknown-unknown leads to a warning instead of an error, while a regular unrecognized driver option leads to an error.
We deprecate driver options and make use of them warnings, and newer Clang generally emits more warnings. These would break -Werror users as well, but we still do them anyway if reasonable.
I understand that it is a small piece of debt, but my point is that we don't need the debt.
+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).