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