[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

Fangrui Song via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 14 15:50:40 PST 2022


MaskRay added a comment.

In D105169#3321237 <https://reviews.llvm.org/D105169#3321237>, @hyeongyukim wrote:

> In D105169#3319773 <https://reviews.llvm.org/D105169#3319773>, @dblaikie wrote:
>
>> In D105169#3315009 <https://reviews.llvm.org/D105169#3315009>, @MaskRay wrote:
>>
>>> It may not be worth changing now, but I want to mention: it's more conventional to have a `BoolOption` which adds `-[no-]noundef-analysis`. Since both positive and negative forms exist. When we make the default switch, existing users don't need to change the option. After the option becomes quite stable and the workaround is deemed not useful, we can remove the CC1 option.
>>
>> +1 to this (changing the name and the default at the same time makes migrations a bit more difficult - if the default is changed without renaming (by having both positive and negative flag names) then users can adopt their current default explicitly with no change ahead of picking up the patch that changes the default) & also this flag seems to have no tests? Could you (@hyeongyukim ) add some frontend test coverage for the flag - and yeah, maybe consider giving it a name that has both explicit on/off names, as @maskray suggested? (I think that's useful even after the default switch - since a user might want to override a previous argument on the command line, etc)
>
> Sure, I'll add some tests.
> By the way, 
is it right to change the flag's name to `-[no-]enable-noundef-analysis`? or would it be better to use `-[no-]noundef-analysis` as @MaskRay suggested?
> I prefer to use `-[no-]enable-noundef-analysis` to maintain backward compatibility.

For driver and CC1 options, the convention of boolean options is to use `[no-]`, not use `enable-` or `disable-`.
That said, `-[no-]enable-noundef-analysis` sounds good to me since `no-noundef-analysis` may be odd and `-enable-noundef-analysis` maintains backward compatibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105169/new/

https://reviews.llvm.org/D105169



More information about the cfe-commits mailing list