Page MenuHomePhabricator

[clang] Add fix-it for -Wreorder-ctor.
Needs ReviewPublic

Authored by adamcz on Mon, Sep 7, 10:03 AM.



This version is very limited. It does not work when comments are present
anywhere in the initializer list, since we do not have a good way to
associate them to specific initalizer and move them around.

The fix-it is associated with the last ctor-order diagnostic and
reorders all initializers. This is better than providing fix for each
diagnostic generated, since user would have to apply them one-by-one.

This also fixes a bug that would trigger an assert() in dependent
classes, when initializer present in code might not be found in the
idealized list of initializers. That is related to PR7179.

Diff Detail

Unit TestsFailed

90 mslinux > Clang.SemaCXX::warn-reorder-ctor-initialization.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -fsyntax-only -Wreorder -verify /mnt/disks/ssd0/agent/llvm-project/clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp
170 mswindows > Clang.SemaCXX::warn-reorder-ctor-initialization.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16n2-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -fsyntax-only -Wreorder -verify C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\SemaCXX\warn-reorder-ctor-initialization.cpp

Event Timeline

adamcz created this revision.Mon, Sep 7, 10:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Sep 7, 10:03 AM
adamcz requested review of this revision.Mon, Sep 7, 10:03 AM
adamcz updated this revision to Diff 290312.Mon, Sep 7, 10:06 AM

fixed comment typo

adamcz updated this revision to Diff 290313.Mon, Sep 7, 10:07 AM

Add a missing const

thanks, comments around some implementations. the only high level question i have is about the choice of location for fix-it (see the detailed comment inline)


nit: assert for fileids of begin and end at the beginning.


since Tok and End are in the same file you can just use < operator.


i don't really follow when this happens. comments mentions a dependent base, but test case only has a templated constructor. why do we think it is safe to continue in this case, while we bail out in case of a dependent class? i would suggest just bailing out at the beginning of the function if constructor is dependent too (in addition to it's class being dependent)


why not move this into previous if block i.e.

if (isoutoforder) {
 if(!Initswithidealindex.empty()) { ... }
 if(IdealIndex== NumIdealInits) { ..}
 if(!InitsWithIdealIndex.empty() {
    // emit diag

I totally agree with having a single fix-it to re-order all of the initializers, but I am not sure if last initializer is the best location for that fixit, especially for interactive tools (it is definitely the right behaviour for command line tools like clang and clang-tidy tho, as they'll either print the fix multiple times or try to apply conflicting fixes)

e.g. if user sees 3 out-of-order initializer warnings, they'll likely hover over one of them and say, "aaah" and fix the code themselves, if fixit wasn't available with that one.
Moreover when there's only 1 out-of-order initialzier warning, they'll see the fixit attached no matter what.
Next time, when there are multiple warnings they won't see it and will likely file bugs saying that "clang(d) doesn't generate fixes all the time".

But I also don't have any better suggestions, as having it multiple times will definitely be helpful for interactive tools but will regress the others.. Maybe attaching it to the first initializer might be better overall?


can we have some tests for this case?


nit: ShouldEmitFix = ShouldEmitFix && !Range...


what if there are comments after last initializer ? I think we should rather use LBraceLoc for the constructor in here.


i think it is enough to have int a; as a member do we really need a separate type? + why don't we just merge this with the previous case?