[PATCH] D135076: [Clang] Make offloading flags accept '-' and '--'
Fangrui Song via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 3 11:39:00 PDT 2022
MaskRay added a comment.
In D135076#3831347 <https://reviews.llvm.org/D135076#3831347>, @jhuber6 wrote:
> In D135076#3831340 <https://reviews.llvm.org/D135076#3831340>, @MaskRay wrote:
>
>> In D135076#3831307 <https://reviews.llvm.org/D135076#3831307>, @jhuber6 wrote:
>>
>>> In D135076#3831298 <https://reviews.llvm.org/D135076#3831298>, @MaskRay wrote:
>>>
>>>> We really want these `--offload-*` users to stick with one canonical form, not `-offload-*` in some places while `--offload-*` in other places.
>>>>
>>>> Another angle is that people find `-offload-*` working with a new clang may try `-offload-*` on an old clang and get `-o ffload-*`.
>>>>
>>>> And `-offload-*` doesn't help misspelled options.
>>>
>>> This is fine, as long as users get more distinct feedback that `-offload-arch` isn't doing what they think it does. Normally we'd get some error and a helpful suggestion if the option is misspelled, but with `-o` options we don't get anything.
>>> My only qualm with the current state is that it's not obvious that `-offload-arch` is actually `-o ffload-arch` for most cases.
>>
>> You can make `-oxxx` an error if offloading is used (`-o xxx` is still allowed). Then no `--offload-*` needs a `-offload-*` form.
>
> The problem here is that the `--offload-*` options are used to enable the offloading toolchain for some targets (e.g. OpenMP). Would a check on `-o' that emits a warning if it matches closely a known option work? E.g. `-offload-arch` would warn that the user may mean `--offload-arch`.
If we don't have `Joined` `-o`, we won't have the option collision problem. But we have `Joined` `-o`, and we should not introduce new aliases to collide with `-o`.
I am fine if `Joined` `-o` can go away but there are too many uses so such a change would be destructive.
My idea is to just disallow `Joined` `-o` when targeting a specific environment (e.g. when offloading toolchain is used).
>> This patch probably provides some convenience but regresses other aspects, and I think it should be reverted.
>
> If you think this should be reverted then I'm fine with it. I would like to see some kind of solution to this problem however, as I have dealt with this problem many times when working with users.
As mentioned, making users stick with more forms instead of all using the canonical form will do harm in other aspects.
I have explained these aspects in my previous comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135076/new/
https://reviews.llvm.org/D135076
More information about the cfe-commits
mailing list