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] ---------------- 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. 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
