Skip to content

Commit

Permalink
[clangd] Dont require confirmation for include-cleaner batch-fixes (#…
Browse files Browse the repository at this point in the history 
…76826)

False negative/positive rate has decreased to the degree that these
extra confirmations no longer provide any value, but only create
friction in the happy case.

When we have false analysis, people usually need to apply the fixes and
run the builds to discover the failure. At that point they can add
relevant IWYU pragmas to guide analysis, and will be more likely to
create bug reports due to extra annoyance :)
  • Loading branch information
@kadircet
kadircet committed Jan 4, 2024
1 parent f5efa74 commit 2336f79
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 99 deletions.
32 changes: 1 addition & 31 deletions clang-tools-extra/clangd/IncludeCleaner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
#include <cassert>
#include <iterator>
#include <map>
#include <memory>
#include <optional>
#include <string>
#include <utility>
Expand Down Expand Up @@ -237,18 +236,6 @@ removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
Diag.Fixes.front().Edits.begin(),
Diag.Fixes.front().Edits.end());
}

// TODO(hokein): emit a suitable text for the label.
ChangeAnnotation Annotation = {/*label=*/"",
/*needsConfirmation=*/true,
/*description=*/""};
static const ChangeAnnotationIdentifier RemoveAllUnusedID =
"RemoveAllUnusedIncludes";
for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
RemoveAll.Edits[I].annotationId = ID;
RemoveAll.Annotations.push_back({ID, Annotation});
}
return RemoveAll;
}

Expand All @@ -268,20 +255,8 @@ addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
Edits.try_emplace(Edit.newText, Edit);
}
}
// FIXME(hokein): emit used symbol reference in the annotation.
ChangeAnnotation Annotation = {/*label=*/"",
/*needsConfirmation=*/true,
/*description=*/""};
static const ChangeAnnotationIdentifier AddAllMissingID =
"AddAllMissingIncludes";
unsigned I = 0;
for (auto &It : Edits) {
ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
for (auto &It : Edits)
AddAllMissing.Edits.push_back(std::move(It.second));
AddAllMissing.Edits.back().annotationId = ID;

AddAllMissing.Annotations.push_back({ID, Annotation});
}
return AddAllMissing;
}
Fix fixAll(const Fix &RemoveAllUnused, const Fix &AddAllMissing) {
Expand All @@ -292,11 +267,6 @@ Fix fixAll(const Fix &RemoveAllUnused, const Fix &AddAllMissing) {
FixAll.Edits.push_back(F);
for (const auto &F : AddAllMissing.Edits)
FixAll.Edits.push_back(F);

for (const auto &A : RemoveAllUnused.Annotations)
FixAll.Annotations.push_back(A);
for (const auto &A : AddAllMissing.Annotations)
FixAll.Annotations.push_back(A);
return FixAll;
}

Expand Down
68 changes: 0 additions & 68 deletions clang-tools-extra/clangd/test/include-cleaner-batch-fix.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,21 +157,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
# CHECK-NEXT: "changeAnnotations": {
# CHECK-NEXT: "AddAllMissingIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "AddAllMissingIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -185,7 +174,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand Down Expand Up @@ -213,29 +201,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
# CHECK-NEXT: "changeAnnotations": {
# CHECK-NEXT: "AddAllMissingIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "AddAllMissingIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -249,7 +218,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -263,7 +231,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -277,7 +244,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand Down Expand Up @@ -342,21 +308,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
# CHECK-NEXT: "changeAnnotations": {
# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -370,7 +325,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand Down Expand Up @@ -398,29 +352,10 @@
# CHECK-NEXT: {
# CHECK-NEXT: "arguments": [
# CHECK-NEXT: {
# CHECK-NEXT: "changeAnnotations": {
# CHECK-NEXT: "AddAllMissingIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "AddAllMissingIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "RemoveAllUnusedIncludes0": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: },
# CHECK-NEXT: "RemoveAllUnusedIncludes1": {
# CHECK-NEXT: "label": "",
# CHECK-NEXT: "needsConfirmation": true
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: "documentChanges": [
# CHECK-NEXT: {
# CHECK-NEXT: "edits": [
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes0",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -434,7 +369,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "RemoveAllUnusedIncludes1",
# CHECK-NEXT: "newText": "",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -448,7 +382,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes0",
# CHECK-NEXT: "newText": "#include {{.*}}bar.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand All @@ -462,7 +395,6 @@
# CHECK-NEXT: }
# CHECK-NEXT: },
# CHECK-NEXT: {
# CHECK-NEXT: "annotationId": "AddAllMissingIncludes1",
# CHECK-NEXT: "newText": "#include {{.*}}foo.h{{.*}}",
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
Expand Down

0 comments on commit 2336f79

Please sign in to comment.