Page MenuHomePhabricator

[tooling] Add support for fixits that indicate code will need reformatting
Needs ReviewPublic

Authored by njames93 on Nov 9 2020, 2:27 PM.

Details

Summary

Implementing a FixItHint which indicates that code needs reformatting but not changing.

This is achieved by creating a FixItHint where the RemoveRange is equal to the InsertFromRange.
This means any dependent clients wont need updating as that is essentially a no-op. For clients that (are updated to) handle it, it signifies that code needs reformatting.
Currently this has been added to clang-tidy but for any refactoring tool that uses the Replacements its rather trivial to include support.
Generally this should be tacked onto other fix-its that maybe span multiple lines or insert/remove braces.

Diff Detail

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

njames93 created this revision.Nov 9 2020, 2:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 9 2020, 2:27 PM
njames93 requested review of this revision.Nov 9 2020, 2:27 PM
jingham added a subscriber: jingham.EditedNov 9 2020, 2:50 PM

IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?

IIUC, the expression parser part of this change suppresses any Fixits that are clang-tidy type rewrites, is that right? If so that is indeed the correct behavior. But the fact that this change implements that behavior is entirely non-obvious at the call site. Could you either add a comment here explaining to point of the test or maybe wrap the test in a method on the FixItHint class so that it's self documenting?

Hmm thats a good point, maybe a method inside FixItHint like,

bool isReformatRange() const {
  return InsertFromRange() == RemoveRange();
}
njames93 updated this revision to Diff 304126.Nov 10 2020, 3:48 AM

Add isReformatFixit method to FixItHint to better express intent.

An example of this in action is D91149

njames93 updated this revision to Diff 304198.Nov 10 2020, 8:12 AM

Teach clangd to ignore these fix-its.

kadircet added inline comments.Nov 11 2020, 1:00 AM
clang-tools-extra/clangd/Diagnostics.cpp
636

in theory yes, as we have access to source manager, we can fetch file contents and create formatted replacements (see cleanupAndFormat). but formatting those fixes can imply significant delays on clangd's diagnostic cycles (if there are many of those), that's the reason why we currently don't format fixits.

637

rather than doing an extra loop, can we just skip those in the for loop below ?

njames93 added inline comments.Nov 11 2020, 3:27 AM
clang-tools-extra/clangd/Diagnostics.cpp
636

I mean if clangd is extended to support async code actions for diagnostics instead of just using the inline extension. Having said that, if that were the case, this would probably not be where that support is implemented.

637

We could, however that would result in messier code further down when trying to build a synthetic message.

njames93 updated this revision to Diff 304477.Nov 11 2020, 4:17 AM
njames93 marked 2 inline comments as done.

Removed the first loop for clangd diagnostic, turns out it didnt make the following code that much messier.

kadircet added inline comments.Nov 11 2020, 4:30 AM
clang-tools-extra/clangd/Diagnostics.cpp
671–672

nit: you can check Edits.size() == 1 here, s/SingleFixIt/FixItForLastEdit and get rid of the optional.

njames93 updated this revision to Diff 304574.Nov 11 2020, 10:35 AM
njames93 marked an inline comment as done.

Address nit by replacing optional.

nridge added a subscriber: nridge.Nov 11 2020, 5:29 PM