aaron.ballman 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: > 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. 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
