hans 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: > 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. 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
