MaskRay 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] ---------------- aaron.ballman wrote: > MaskRay wrote: > > aaron.ballman wrote: > > > hans wrote: > > > > MaskRay wrote: > > > > > dblaikie wrote: > > > > > > 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. > > > > > (I was expecting more reasoning than "it's substantial technical > > > > > debt, so we take it".) > > > > > > > > > > I have performed a survey on existing `Wno-overriding-t-option` uses. > > > > > Only some Darwin use cases like > > > > > https://github.com/oldzhu/4dotnet/blob/master/package/dotnetcore/dotnetruntime/modified/configurecompiler.cmake.v6.0.2#L404 > > > > > requires some thoughts. It may be on the boundary of the scale that > > > > > I'd consider a workaround. Hence one of my previous comments said: > > > > > > > > > > > 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: > > > > > > > > > > If we rename `warn_drv_overriding_t_option` below and clarify the > > > > > comment, I think the concern of new uses adopting > > > > > `warn_drv_overriding_t_option` (to-be-renamed) will be very low. > > > > > ``` > > > > > // Don't use warn_drv_overriding_t_option for new diagnostics. > > > > > def warn_drv_overriding_t_option : Warning< > > > > > "overriding '%0' option with '%1'">, > > > > > InGroup<OverridingTOption>; > > > > > def warn_drv_overriding_option : Warning< > > > > > "overriding '%0' option with '%1'">, > > > > > InGroup<OverridingOption>; > > > > > ``` > > > > > > > > > > Essentially, we split the `overriding-option` group and make the > > > > > Darwin use its own group. > > > > > > > > > > > With this patch: > > > > > > > > > > > > * Some current users of overriding-t-option will need to migrate to > > > > > > overriding-option; others will not. > > > > > > > > > > Yes. > > > > > > > > > > > * 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. > > > > > > > > > > Yes. > > > > > > > > > > > The difference between which are canonically "t" (or not) is an > > > > > > accident of history and will be hard to reason about. > > > > > > > > > > If we add `-Wno-overriding-darwin-option` as the canonical spelling, > > > > > this concern can be addressed. > > > > > > > > > > > * 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. > > > > > > > > > > Yes. > > > > > I do not want -Wno-overriding-t-option (even it is hidden) to affect > > > > > good uses while an alias. > > > > > > > > This seems to be the core of the disagreement. Could you expand on why > > > > this is important? > > > > 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. > > > > > > Assuming the analysis from @dexonsmith is accurate (thank you, that was a > > > really handy summary!), I think going with an alias is a good tradeoff > > > between maintenance burden and user experience, but I'd also like to > > > understand this point better: > > > > > > > I do not want -Wno-overriding-t-option (even it is hidden) to affect > > > > good uses while an alias. > > > > > > If we eventually get rid of the alias, then I can see why this would be a > > > problem (particularly for `-Werror` users), but... I don't imagine a need > > > to get rid of the alias. At most, I think we'd want to remove any mention > > > of the alias from documentation, etc and so over time search engines will > > > "forget" about it and so users won't organically run into the `-t-` alias. > > >> I do not want -Wno-overriding-t-option (even it is hidden) to affect > > >> good uses while an alias. > > > > > > This seems to be the core of the disagreement. Could you expand on why > > > this is important? > > > > > I think going with an alias is a good tradeoff between maintenance > > > burden and user experience, but I'd also like to understand this point > > > better: > > > > This thought comes from my view of previous discussions regarding improved > > diagnostics break `-Werror` users. Frankly, I believe there are numerous > > other diagnostics that are hundred-time more disruptive than what we are > > currently observing as a relatively minor disturbance. Thus, I am having > > difficulty comprehending the rationale behind wanting to maintain this > > workaround indefinitely. > For me, I think `-Werror` is a red herring -- users who want warnings to be > errors are explicitly asking to make compiler upgrades more disruptive > because compiler upgrades *always* come with new/different diagnostic > behaviors. So I don't consider `-Werror` breaking the user to be an issue; > we're doing what they asked us to do. We shouldn't make that more painful > than it needs to be, but that's all. > > What compels me are users who aren't using `-Werror` and do the compiler > upgrade. Some class of users aren't going to see that this option has been > renamed, it will simply stop working for them and they possibly won't even > notice it. Adding an alias means these folks don't run into as many problems > as they otherwise would, though supporting these users does effectively mean > we support the option indefinitely. But as I understand it, the cost to the > community is minimal (approx one line of code to add the alias and a test > case proving the alias works), so I'm not certain what maintenance burden you > have in mind. I am fine with a temporary alias, but I am not comfortable for a permanent alias. I have analyzed numerous `-Wno-overriding-t-option` uses and conclude that they deserve a warning (a very small number of -ffp-model= related users deserve it as well, though I agree our -ffp-model= warning model is a bit stronger our practice for other options). With ToT, using `-Wno-overriding-t-option` gives this typo correction ``` % fclang -Wno-overriding-t-option -c a.c warning: unknown warning option '-Wno-overriding-t-option'; did you mean '-Wno-overriding-option'? [-Wunknown-warning-option] 1 warning generated. ``` So users noticing the warning will know what to do, if they decide to silence it. By reading the cdb5240287897ff7649d40e3752ecf23e50b57f5 change, I can see an attempt to make the warning preciser, but I do not see an attempt to support `-Werror` users having the conflicting target triple and `-m<os>-version-min=`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158301/new/ https://reviews.llvm.org/D158301 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
