This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add fixit for Wreorder-ctor
ClosedPublic

Authored by njames93 on Mar 16 2021, 3:40 PM.

Details

Summary

Create fix-it hints to fix the order of constructors.
To make this a lot simpler, I've grouped all the warnings for each out of order initializer into 1.
This is necessary as fixing one initializer would often interfere with other initializers.

Diff Detail

Event Timeline

njames93 created this revision.Mar 16 2021, 3:40 PM
njames93 requested review of this revision.Mar 16 2021, 3:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 3:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 updated this revision to Diff 331581.Mar 18 2021, 9:03 AM

Fix formatting issues

njames93 updated this revision to Diff 331582.Mar 18 2021, 9:03 AM

Arc got confused

aaron.ballman added inline comments.Mar 18 2021, 10:03 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8611 ↗(On Diff #331582)

Hmmm, how about initializer order does not match the declaration order or something along those lines? The "some" and "in the correct order" feel a bit hand-wavy for a warning.

clang/lib/Sema/SemaDeclCXX.cpp
5242 ↗(On Diff #331582)
5247 ↗(On Diff #331582)
5304 ↗(On Diff #331582)
njames93 marked 4 inline comments as done.Mar 18 2021, 11:20 AM
njames93 added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
5242 ↗(On Diff #331582)

This was copied from the old impl, Not sure it this is going to change behaviour though.
No tests needed to be updated though,

njames93 updated this revision to Diff 331635.Mar 18 2021, 11:20 AM
njames93 marked an inline comment as done.

Address nits.

aaron.ballman accepted this revision.Mar 18 2021, 11:25 AM

Thanks for this! The changes LGTM, but wait to land it for a day or two in case @rsmith has concerns.

clang/lib/Sema/SemaDeclCXX.cpp
5242 ↗(On Diff #331582)

The diagnostic engine knows how to format anything that's a NamedDecl (including quoting it properly, etc), but it does the same work for DeclarationName. There shouldn't be a change in behavior in this case, just a slight simplification of code.

This revision is now accepted and ready to land.Mar 18 2021, 11:25 AM
This revision was automatically updated to reflect the committed changes.