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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
