Page MenuHomePhabricator

[clang-tidy] Handled insertion only fixits when determining conflicts.
ClosedPublic

Authored by njames93 on Jun 30 2020, 11:25 AM.

Details

Summary

Handle insertion fix-its when removing incompatible errors by introducting a new EventType ET_Insert
This has lower prioirty than End events, but higher than begin.
Idea being If an insert is at the same place as a begin event, the insert should be processed first to reduce unnecessary conflicts.
Likewise if its at the same place as an end event, process the end event first for the same reason.

This also fixes https://bugs.llvm.org/show_bug.cgi?id=46511.

Diff Detail

Event Timeline

njames93 created this revision.Jun 30 2020, 11:25 AM
njames93 marked an inline comment as done.Jun 30 2020, 11:29 AM

I've not added any specific unit tests for this, Don't think there would be much gained from them.
The init-variables and isolate-declaration checks do a good enough job of demonstrating the fix is working.
If needs must specific checks will be added.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-init-variables-conflict.cpp
16

I'll sort that out next diff

njames93 added a project: Restricted Project.Jun 30 2020, 11:37 AM
njames93 updated this revision to Diff 281194.Jul 28 2020, 5:16 AM

Fix new lines in test

njames93 updated this revision to Diff 281200.Jul 28 2020, 5:39 AM

Replace if/else logic with switches

aaron.ballman accepted this revision.Jul 28 2020, 11:38 AM

LGTM with a few style nits.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
629

I'd drop these return statements and the unreachable, below. The switch is fully-covered, so we'll get a diagnostic if any new enumerations are added and we forget to update this switch.

709

Similar here with continue and the unreachable.

This revision is now accepted and ready to land.Jul 28 2020, 11:38 AM
njames93 updated this revision to Diff 281418.Jul 28 2020, 4:45 PM
njames93 marked 2 inline comments as done.

Remove unreachable after switch

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
629

I'd still need a break anyway, but yes the unreachable probably isn't required

709

As this is in a loop, I'm torn whether using break makes it less readable. WDYT?

aaron.ballman accepted this revision.Jul 29 2020, 4:47 AM

LG!

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
709

I tend to prefer break to continue because it's terribly easy to miss the fact that the switch statement is continuing the loop should someone decide they want to stick some code below the switch but within the loop for some reason.