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

Tue Aug 22 14:44:04 PDT 2023

dblaikie 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:
> > > 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.
> Thanks for taking time to write the summary. I agree with the analysis and sorry that this discussion has taken your valuable time.
> 
> > 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.
> 
> This patch wasn't created with a good motivation. It was for discussion when people raised compatibility concern (valid) that I don't agree with, considering the scope of affected users and how reasonable the `-Wno-overriding-t-option` use is.
> 
> I do not want `-Wno-overriding-t-option` (even it is hidden) to affect good uses while an alias. I think I am happy with an alias that will be removed, say one year.
> 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.

(really appreciate your work, @dexonsmith, btw - both having historic context, and any help out with review load, etc, is really valuable)

Yeah, I'd rather see this as an alias. I don't feel like it's worth removing later, though. I don't think it's substantial technical debt to keep an old alias around. It doesn't add significant friction to the project that I can think of.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158301