[PATCH] D52264: Deduplicate replacements from diagnostics.
ioeric added a subscriber: bkramer. ioeric added a comment. I wasn't aware of the bug. I have just replied to the issue. Thanks for letting me know! Repository: rL LLVM https://reviews.llvm.org/D52264 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D52264: Deduplicate replacements from diagnostics.
I wasn't aware of the bug. I have just replied to the issue. Thanks for letting me know! On Tue, Sep 25, 2018 at 10:44 AM Stephen Kelly via Phabricator < revi...@reviews.llvm.org> wrote: > steveire added a comment. > > Was this motivated by https://bugs.llvm.org/show_bug.cgi?id=38910 ? > > > Repository: > rL LLVM > > https://reviews.llvm.org/D52264 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52264: Deduplicate replacements from diagnostics.
steveire added a comment. Was this motivated by https://bugs.llvm.org/show_bug.cgi?id=38910 ? Repository: rL LLVM https://reviews.llvm.org/D52264 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52264: Deduplicate replacements from diagnostics.
This revision was automatically updated to reflect the committed changes. Closed by commit rL342951: Deduplicate replacements from diagnostics. (authored by ioeric, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D52264?vs=166112&id=166826#toc Repository: rL LLVM https://reviews.llvm.org/D52264 Files: clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file2.yaml clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/identical.cpp clang-tools-extra/trunk/test/clang-apply-replacements/identical.cpp Index: clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp === --- clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp +++ clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp @@ -125,7 +125,8 @@ } /// \brief Extract replacements from collected TranslationUnitReplacements and -/// TranslationUnitDiagnostics and group them per file. +/// TranslationUnitDiagnostics and group them per file. Identical replacements +/// from diagnostics are deduplicated. /// /// \param[in] TUs Collection of all found and deserialized /// TranslationUnitReplacements. @@ -142,10 +143,20 @@ llvm::DenseMap> GroupedReplacements; - auto AddToGroup = [&](const tooling::Replacement &R) { + // Deduplicate identical replacements in diagnostics. + // FIXME: Find an efficient way to deduplicate on diagnostics level. + llvm::DenseMap> + DiagReplacements; + + auto AddToGroup = [&](const tooling::Replacement &R, bool FromDiag) { // Use the file manager to deduplicate paths. FileEntries are // automatically canonicalized. if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) { + if (FromDiag) { +auto &Replaces = DiagReplacements[Entry]; +if (!Replaces.insert(R).second) + return; + } GroupedReplacements[Entry].push_back(R); } else if (Warned.insert(R.getFilePath()).second) { errs() << "Described file '" << R.getFilePath() @@ -155,13 +166,13 @@ for (const auto &TU : TUs) for (const tooling::Replacement &R : TU.Replacements) - AddToGroup(R); + AddToGroup(R, false); for (const auto &TU : TUDs) for (const auto &D : TU.Diagnostics) for (const auto &Fix : D.Fix) for (const tooling::Replacement &R : Fix.second) - AddToGroup(R); + AddToGroup(R, true); // Sort replacements per file to keep consistent behavior when // clang-apply-replacements run on differents machine. Index: clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file2.yaml === --- clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file2.yaml +++ clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file2.yaml @@ -0,0 +1,14 @@ +--- +MainSourceFile: identical.cpp +Diagnostics: + - DiagnosticName: test-identical-insertion +Message: Fix +FilePath: $(path)/identical.cpp +FileOffset: 12 +Replacements: + - FilePath:$(path)/identical.cpp +Offset: 12 +Length: 0 +ReplacementText: '0' +... + Index: clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml === --- clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml +++ clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml @@ -10,9 +10,5 @@ Offset: 12 Length: 0 ReplacementText: '0' - - FilePath:$(path)/identical.cpp -Offset: 12 -Length: 0 -ReplacementText: '0' ... Index: clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/identical.cpp === --- clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/identical.cpp +++ clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/identical.cpp @@ -1,2 +1,2 @@ class MyType {}; -// CHECK: class MyType00 {}; +// CHECK: class MyType0 {}; Index: clang-tools-extra/trunk/test/clang-apply-replacements/identical.cpp === --- clang-tools-extra/trunk/test/clang-apply-replacements/identical.cpp +++ clang-tools-extra/trunk/test/clang-apply-replacements/identical.cpp @@ -1,5 +1,6 @@ // RUN: mkdir -p %T/Inputs/identical // RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical/identical.cpp > %T/Inputs/identi
[PATCH] D52264: Deduplicate replacements from diagnostics.
bkramer accepted this revision. bkramer added a comment. This revision is now accepted and ready to land. lg Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52264 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52264: Deduplicate replacements from diagnostics.
ioeric created this revision. ioeric added a reviewer: bkramer. Herald added a subscriber: cfe-commits. After r329813, clang-apply-replacements stopped deduplicating identical replacements; however, tools like clang-tidy relies on the deduplication of identical dignostics replacements from different TUs to apply fixes correctly. This change partially roll back the behavior by deduplicating changes from diagnostics. Ideally, we should deduplicate on diagnostics level, but we need to figure out an effecient way. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52264 Files: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp test/clang-apply-replacements/Inputs/identical/file1.yaml test/clang-apply-replacements/Inputs/identical/file2.yaml test/clang-apply-replacements/Inputs/identical/identical.cpp test/clang-apply-replacements/identical.cpp Index: test/clang-apply-replacements/identical.cpp === --- test/clang-apply-replacements/identical.cpp +++ test/clang-apply-replacements/identical.cpp @@ -1,5 +1,6 @@ // RUN: mkdir -p %T/Inputs/identical // RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical/identical.cpp > %T/Inputs/identical/identical.cpp // RUN: sed "s#\$(path)#%/T/Inputs/identical#" %S/Inputs/identical/file1.yaml > %T/Inputs/identical/file1.yaml +// RUN: sed "s#\$(path)#%/T/Inputs/identical#" %S/Inputs/identical/file2.yaml > %T/Inputs/identical/file2.yaml // RUN: clang-apply-replacements %T/Inputs/identical // RUN: FileCheck -input-file=%T/Inputs/identical/identical.cpp %S/Inputs/identical/identical.cpp Index: test/clang-apply-replacements/Inputs/identical/identical.cpp === --- test/clang-apply-replacements/Inputs/identical/identical.cpp +++ test/clang-apply-replacements/Inputs/identical/identical.cpp @@ -1,2 +1,2 @@ class MyType {}; -// CHECK: class MyType00 {}; +// CHECK: class MyType0 {}; Index: test/clang-apply-replacements/Inputs/identical/file2.yaml === --- test/clang-apply-replacements/Inputs/identical/file2.yaml +++ test/clang-apply-replacements/Inputs/identical/file2.yaml @@ -10,9 +10,5 @@ Offset: 12 Length: 0 ReplacementText: '0' - - FilePath:$(path)/identical.cpp -Offset: 12 -Length: 0 -ReplacementText: '0' ... Index: test/clang-apply-replacements/Inputs/identical/file1.yaml === --- test/clang-apply-replacements/Inputs/identical/file1.yaml +++ test/clang-apply-replacements/Inputs/identical/file1.yaml @@ -10,9 +10,5 @@ Offset: 12 Length: 0 ReplacementText: '0' - - FilePath:$(path)/identical.cpp -Offset: 12 -Length: 0 -ReplacementText: '0' ... Index: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp === --- clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp +++ clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp @@ -125,7 +125,8 @@ } /// \brief Extract replacements from collected TranslationUnitReplacements and -/// TranslationUnitDiagnostics and group them per file. +/// TranslationUnitDiagnostics and group them per file. Identical replacements +/// from diagnostics are deduplicated. /// /// \param[in] TUs Collection of all found and deserialized /// TranslationUnitReplacements. @@ -142,10 +143,20 @@ llvm::DenseMap> GroupedReplacements; - auto AddToGroup = [&](const tooling::Replacement &R) { + // Deduplicate identical replacements in diagnostics. + // FIXME: Find an efficient way to deduplicate on diagnostics level. + llvm::DenseMap> + DiagReplacements; + + auto AddToGroup = [&](const tooling::Replacement &R, bool FromDiag) { // Use the file manager to deduplicate paths. FileEntries are // automatically canonicalized. if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) { + if (FromDiag) { +auto &Replaces = DiagReplacements[Entry]; +if (!Replaces.insert(R).second) + return; + } GroupedReplacements[Entry].push_back(R); } else if (Warned.insert(R.getFilePath()).second) { errs() << "Described file '" << R.getFilePath() @@ -155,13 +166,13 @@ for (const auto &TU : TUs) for (const tooling::Replacement &R : TU.Replacements) - AddToGroup(R); + AddToGroup(R, false); for (const auto &TU : TUDs) for (const auto &D : TU.Diagnostics) for (const auto &Fix : D.Fix) for (const tooling::Replacement &R : Fix.second) - AddToGroup(R); + AddToGroup(R, true); // Sort replacements per file to keep consistent behavior when // clang-apply-re