[PATCH] D158301: Add back overriding-t-options for -m<os>-version-min diagnostic

Tue Aug 22 12:56:03 PDT 2023

dexonsmith added inline comments.

================
Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]

----------------
MaskRay wrote:
> dexonsmith wrote:
> > MaskRay wrote:
> > > hans wrote:
> > > > MaskRay wrote:
> > > > > hans wrote:
> > > > > > dexonsmith wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > Why would we want to use the old name here? An alias seems strictly better to me. 
> > > > > > > > Making `overriding-t-option` an alias for `overriding-option` would make `-Wno-overriding-t-option` applies to future overriding option diagnostics, which is exactly what I want to avoid.
> > > > > > > > 
> > > > > > > I understand that you don't want `-t-` to apply to work on future overriding option diagnostics, but I think the platform divergence you're adding here is worse.
> > > > > > > 
> > > > > > > Having a few Darwin-specific options use `-Woverriding-t-option` (and everything else use `-Woverriding-option`) as the canonical spelling is hard to reason about for maintainers, and for users.
> > > > > > > 
> > > > > > > And might not users on other platforms have `-Woverriding-t-option` hardcoded in  build settings? (So @dblaikie's argument that we shouldn't arbitrarily make things hard for users would apply to all options?)
> > > > > > > 
> > > > > > > IMO, if we're not comfortable removing `-Woverriding-t-option` entirely, then it should live on as an alias (easy to reason about), not as canonical-in-special-cases (hard to reason about).
> > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option entirely, then it should live on as an alias (easy to reason about), not as canonical-in-special-cases (hard to reason about).
> > > > > > 
> > > > > > +1 if we can't drop the old spelling, an alias seems like the best option.
> > > > > Making `overriding-t-option` an alias for `overriding-option`, as I mentioned, will make `-Wno-overriding-t-option` affect new overriding-options uses. This is exactly what I want to avoid.
> > > > > 
> > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, they are far fewer than other diagnostics we are introducing or changing in Clang. I can understand the argument "make -Werror users easier for this specific diagnostic" (but `-Werror` will complain about other new diagnostics), but do we really want to in the Darwin case? I think no. They can remove the version from the target triple like https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50　or make the version part consistent with `-m.*os-version-min`.
> > > > > 
> > > > > This change may force these users to re-think how they should fix  their build. I apology to these users, but I don't feel that adding an alias is really necessary.
> > > > > Making overriding-t-option an alias for overriding-option, as I mentioned, will make -Wno-overriding-t-option affect new overriding-options uses. This is exactly what I want to avoid.
> > > > 
> > > > Is keeping them separate actually important, though? -Wno-overriding-option has the same issue in that case, that using the flag will also affect any new overriding-options uses, and I don't think that's a problem.
> > > `-Wno-overriding-option` is properly named, so affecting new overriding-options uses looks fine to me.
> > > `-Wno-overriding-t-option` is awkward, and making it affect new uses makes me nervous.
> > > 
> > > The gist of my previous comment is whether the uses cases really justify a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > > This change is not about changing a semantic warning that has mixed opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not justified).
> > > The gist of my previous comment is whether the uses cases really justify a tiny bit of tech bit in clang and I think the answer is no.
> > > 
> > 
> > I think we agree that we should add the minimal technical debt to clang.
> > 
> > This patch is harder-to-reason about, and thus bigger IMO, technical debt than adding an alias would be.
> Honestly when people asked whether we need back compatibility for `-Werror` uses. I disagree with the idea after considering the number of uses and legitimate uses. I've well summarized them up-thread.
> 
> Making overriding-option a super set of overriding-t-option is IMHO the only solution to make -Wno-overriding-t-option not affect other uses, which is what I strive to achieve.
> 
> If `-Woverriding-t-option` looks strange for the Darwin diagnostic and we really want to work around such `-Werror` users (I disagree as I mentioned), we could rename it to something like `-Woverriding-darwin-option` or something else, and add `-Woverriding-t-option` as an alias. Then the diagnostic becomes:
> 
> > overriding '-mmacos-version-min=10.6' option with '-target x86_64-apple-macos10.11.2' [-Woverriding-darwin-option]
> 
> This would still achieve my goal of not making `overriding-t-option` affect `overriding-option`.
> 
> My most honest thinking is that we don't need any of the `overriding-t-option` tech debt. The users need to migrate. It's some work and I apologize to these users, but I don't think these uses are anything close to reasonable that justifies any debt on the clang side.
> Making overriding-option a super set of overriding-t-option is IMHO the only solution to make -Wno-overriding-t-option not affect other uses, which is what I strive to achieve.
> 

It's not clear why this specific piece matters. It seems moot to me. Any current users of overriding-t-option will blindly switch to the new spelling and, in effect, their old uses of `-Woverriding-t-option` [sic] will affect new instances of overriding-option.

Stepping back, here's what I think the effects of the three choices are.

With ToT:
- Current users of overriding-t-option will need to migrate to overriding-option. Whatever their reasons for having overriding-t-option, existing uses will blindly migrate to overriding-option, and thus blindly affect all future overriding-option diagnostics.
- Diagnostics will report as `-Woverriding-option`. Anyone seeing a new diagnostic will use the new spelling.
- Maintainers don't have to think about overriding-t-option anymore, except for supporting user migration.

With an alias:
- Current users of overriding-t-option will not need to migrate to overriding-option. Just like ToT, their existing uses will blindly affect all overriding-option diagnostics.
- Diagnostics will report as `-Woverriding-option`. Anyone seeing a new diagnostic will use the new spelling.
- Maintainers don't have to think about overriding-t-option anymore; it'll be clear that it's just an old spelling.

With this patch:
- Some current users of overriding-t-option will need to migrate to overriding-option; others will not.
- Some diagnostics will report as `-Woverriding-option`, others as `-Woverriding-t-option`, so new users hitting the latter will continue to add the old spelling to build settings.
- The difference between which are canonically "t" (or not) is an accident of history and will be hard to reason about.
- Maintainers who overriding-t-option will be tempted to clean it up, and need to dig up this thread to understand why it's like this, or land a change and hit the same problems.

Do you agree with these effects? If not, what part have I got wrong? Or have I missed another important effect?

If you agree that I have the effects correct, then I'm still confused as to how this patch would be easier to maintain or better for users than an alias.

Note that my personal stake in this is low. My only current involvement in LLVM is volunteering my time as a reviewer. If those with users (e.g., @dblaikie or @aaron.ballman, who added post-commit review to https://reviews.llvm.org/D158137) agree with you that this patch is the right way forward, I'm happy to let it go through.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301