[PATCH] D130678: [clang-tools-extra] Refactor to reduce code duplication

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin created this revision.
Herald added a project: All.
prehistoric-penguin requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130678

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -37,9 +37,11 @@
 namespace clang {
 namespace replace {
 
-std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUReplacements &TUs,
-TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+template 
+std::error_code
+collectReplacementsFromDirectoryImpl(const llvm::StringRef Directory, T &TUs,
+ TUReplacementFiles &TUFiles,
+ clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -67,7 +69,7 @@
 }
 
 yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitReplacements TU;
+typename T::value_type TU;
 YIn >> TU;
 if (YIn.error()) {
   // File doesn't appear to be a header change description. Ignore it.
@@ -82,47 +84,17 @@
 }
 
 std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUDiagnostics &TUs,
+const llvm::StringRef Directory, TUReplacements &TUs,
 TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
-  using namespace llvm::sys::fs;
-  using namespace llvm::sys::path;
-
-  std::error_code ErrorCode;
-
-  for (recursive_directory_iterator I(Directory, ErrorCode), E;
-   I != E && !ErrorCode; I.increment(ErrorCode)) {
-if (filename(I->path())[0] == '.') {
-  // Indicate not to descend into directories beginning with '.'
-  I.no_push();
-  continue;
-}
-
-if (extension(I->path()) != ".yaml")
-  continue;
-
-TUFiles.push_back(I->path());
-
-ErrorOr> Out =
-MemoryBuffer::getFile(I->path());
-if (std::error_code BufferError = Out.getError()) {
-  errs() << "Error reading " << I->path() << ": " << BufferError.message()
- << "\n";
-  continue;
-}
-
-yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitDiagnostics TU;
-YIn >> TU;
-if (YIn.error()) {
-  // File doesn't appear to be a header change description. Ignore it.
-  continue;
-}
-
-// Only keep files that properly parse.
-TUs.push_back(TU);
-  }
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
+}
 
-  return ErrorCode;
+std::error_code collectReplacementsFromDirectory(
+const llvm::StringRef Directory, TUDiagnostics &TUs,
+TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
 }
 
 /// Extract replacements from collected TranslationUnitReplacements and


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -37,9 +37,11 @@
 namespace clang {
 namespace replace {
 
-std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUReplacements &TUs,
-TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+template 
+std::error_code
+collectReplacementsFromDirectoryImpl(const llvm::StringRef Directory, T &TUs,
+ TUReplacementFiles &TUFiles,
+ clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -67,7 +69,7 @@
 }
 
 yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitReplacements TU;
+typename T::value_type TU;
 YIn >> TU;
 if (YIn.error()) {
   // File doesn't appear to be a header change description. Ignore it.
@@ -82,47 +84,17 @@
 }
 
 std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUDiagnostics &TUs,
+const llvm::StringRef Directory, TUReplacements &TUs,
 TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
-  using namespace llvm::sys::fs;
-  using namespace llvm::sys::path;
-
-  std::error_code ErrorCode;
-
-  for (rec

[PATCH] D130678: [clang-tools-extra] Refactor to reduce code duplication

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 448240.
prehistoric-penguin added a comment.

Refine commit msg


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130678/new/

https://reviews.llvm.org/D130678

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -37,9 +37,11 @@
 namespace clang {
 namespace replace {
 
-std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUReplacements &TUs,
-TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+template 
+std::error_code
+collectReplacementsFromDirectoryImpl(const llvm::StringRef Directory, T &TUs,
+ TUReplacementFiles &TUFiles,
+ clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -67,7 +69,7 @@
 }
 
 yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitReplacements TU;
+typename T::value_type TU;
 YIn >> TU;
 if (YIn.error()) {
   // File doesn't appear to be a header change description. Ignore it.
@@ -82,47 +84,17 @@
 }
 
 std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUDiagnostics &TUs,
+const llvm::StringRef Directory, TUReplacements &TUs,
 TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
-  using namespace llvm::sys::fs;
-  using namespace llvm::sys::path;
-
-  std::error_code ErrorCode;
-
-  for (recursive_directory_iterator I(Directory, ErrorCode), E;
-   I != E && !ErrorCode; I.increment(ErrorCode)) {
-if (filename(I->path())[0] == '.') {
-  // Indicate not to descend into directories beginning with '.'
-  I.no_push();
-  continue;
-}
-
-if (extension(I->path()) != ".yaml")
-  continue;
-
-TUFiles.push_back(I->path());
-
-ErrorOr> Out =
-MemoryBuffer::getFile(I->path());
-if (std::error_code BufferError = Out.getError()) {
-  errs() << "Error reading " << I->path() << ": " << BufferError.message()
- << "\n";
-  continue;
-}
-
-yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitDiagnostics TU;
-YIn >> TU;
-if (YIn.error()) {
-  // File doesn't appear to be a header change description. Ignore it.
-  continue;
-}
-
-// Only keep files that properly parse.
-TUs.push_back(TU);
-  }
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
+}
 
-  return ErrorCode;
+std::error_code collectReplacementsFromDirectory(
+const llvm::StringRef Directory, TUDiagnostics &TUs,
+TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
 }
 
 /// Extract replacements from collected TranslationUnitReplacements and


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -37,9 +37,11 @@
 namespace clang {
 namespace replace {
 
-std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUReplacements &TUs,
-TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+template 
+std::error_code
+collectReplacementsFromDirectoryImpl(const llvm::StringRef Directory, T &TUs,
+ TUReplacementFiles &TUFiles,
+ clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -67,7 +69,7 @@
 }
 
 yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitReplacements TU;
+typename T::value_type TU;
 YIn >> TU;
 if (YIn.error()) {
   // File doesn't appear to be a header change description. Ignore it.
@@ -82,47 +84,17 @@
 }
 
 std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUDiagnostics &TUs,
+const llvm::StringRef Directory, TUReplacements &TUs,
 TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
-  using namespace llvm::sys::fs;
-  using namespace llvm::sys::path;
-
-  std::error_code ErrorCode;
-
-  for (recursive_directory_iterator I(

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin created this revision.
prehistoric-penguin added reviewers: Sockke, avogelsgesang, njames93, 
harlanhaskins, hokein, alexfh.
Herald added a project: All.
prehistoric-penguin requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The change will save us one find call in map, which is more efficient and
more clear.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 449569.
prehistoric-penguin added a comment.

Fix per comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 449570.
prehistoric-penguin added a comment.

Update commit message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,12 +135,11 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU) {
   return;
+}
   }
   GroupedReplacements[*Entry].push_back(R);
 } else if (Warned.insert(R.getFilePath()).second) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin added inline comments.



Comment at: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:140
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second) {
   return;

avogelsgesang wrote:
> Previously, we had an additional check `It->second != SourceTU` here. I don't 
> understand what this check was doing, but this does not seem to be a pure 
> refactoring.
> 
> If this is still an NFC change, please update the commit message to explain 
> why this condition was unneeded
Thanks! I have reviewed the change and fixed it, now the logic is aligned


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 449572.
prehistoric-penguin added a comment.

Format change


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,11 +135,9 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -135,11 +135,9 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [NFC][clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 449579.
prehistoric-penguin added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  llvm/include/llvm/ADT/Twine.h

Index: llvm/include/llvm/ADT/Twine.h
===
--- llvm/include/llvm/ADT/Twine.h
+++ llvm/include/llvm/ADT/Twine.h
@@ -291,9 +291,9 @@
 /// Construct from an std::string_view by converting it to a pointer and
 /// length.  This handles string_views on a pure API basis, and avoids
 /// storing one (or a pointer to one) inside a Twine, which avoids problems
-/// when mixing code compiled under various C++ standards.
-/*implicit*/ Twine(const std::string_view &Str)
-: LHSKind(PtrAndLengthKind) {
+/// when mixing code compiled under various C++ standards. Pass string_view
+/// by value is preferred.
+/*implicit*/ Twine(std::string_view Str) : LHSKind(PtrAndLengthKind) {
   LHS.ptrAndLength.ptr = Str.data();
   LHS.ptrAndLength.length = Str.length();
   assert(isValid() && "Invalid twine!");
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -37,9 +37,11 @@
 namespace clang {
 namespace replace {
 
-std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUReplacements &TUs,
-TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+template 
+std::error_code
+collectReplacementsFromDirectoryImpl(const llvm::StringRef Directory, T &TUs,
+ TUReplacementFiles &TUFiles,
+ clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
@@ -67,7 +69,7 @@
 }
 
 yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitReplacements TU;
+typename T::value_type TU;
 YIn >> TU;
 if (YIn.error()) {
   // File doesn't appear to be a header change description. Ignore it.
@@ -82,47 +84,17 @@
 }
 
 std::error_code collectReplacementsFromDirectory(
-const llvm::StringRef Directory, TUDiagnostics &TUs,
+const llvm::StringRef Directory, TUReplacements &TUs,
 TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
-  using namespace llvm::sys::fs;
-  using namespace llvm::sys::path;
-
-  std::error_code ErrorCode;
-
-  for (recursive_directory_iterator I(Directory, ErrorCode), E;
-   I != E && !ErrorCode; I.increment(ErrorCode)) {
-if (filename(I->path())[0] == '.') {
-  // Indicate not to descend into directories beginning with '.'
-  I.no_push();
-  continue;
-}
-
-if (extension(I->path()) != ".yaml")
-  continue;
-
-TUFiles.push_back(I->path());
-
-ErrorOr> Out =
-MemoryBuffer::getFile(I->path());
-if (std::error_code BufferError = Out.getError()) {
-  errs() << "Error reading " << I->path() << ": " << BufferError.message()
- << "\n";
-  continue;
-}
-
-yaml::Input YIn(Out.get()->getBuffer(), nullptr, &eatDiagnostics);
-tooling::TranslationUnitDiagnostics TU;
-YIn >> TU;
-if (YIn.error()) {
-  // File doesn't appear to be a header change description. Ignore it.
-  continue;
-}
-
-// Only keep files that properly parse.
-TUs.push_back(TU);
-  }
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
+}
 
-  return ErrorCode;
+std::error_code collectReplacementsFromDirectory(
+const llvm::StringRef Directory, TUDiagnostics &TUs,
+TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
+  return collectReplacementsFromDirectoryImpl(Directory, TUs, TUFiles,
+  Diagnostics);
 }
 
 /// Extract replacements from collected TranslationUnitReplacements and
@@ -163,11 +135,9 @@
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->secon

[PATCH] D131046: [NFC][clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin updated this revision to Diff 449581.
prehistoric-penguin added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -168,11 +168,9 @@
 if (auto Entry = SM.getFileManager().getFile(Path)) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -168,11 +168,9 @@
 if (auto Entry = SM.getFileManager().getFile(Path)) {
   if (SourceTU) {
 auto &Replaces = DiagReplacements[*Entry];
-auto It = Replaces.find(R);
-if (It == Replaces.end())
-  Replaces.emplace(R, SourceTU);
-else if (It->second != SourceTU)
-  // This replacement is a duplicate of one suggested by another TU.
+auto InsertPos = Replaces.try_emplace(R, SourceTU);
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second && InsertPos.first->second != SourceTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131046: [NFC][clang-tools-extra]Replace find/insert with try_emplace

[ppenguin via Phabricator via cfe-commits]

prehistoric-penguin added a comment.

CI failed since LLVM are still using c++14 to build

  CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ 
-DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-I/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/clang-apply-replacements
 
-I/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements
 -I/var/lib/buildkite-agent/builds/llvm-project/clang/include 
-I/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/include 
-I/var/lib/buildkite-agent/builds/llvm-project/build/include 
-I/var/lib/buildkite-agent/builds/llvm-project/llvm/include 
-I/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/include
 -gmlt -fPIC -fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
-Wmisleading-indentation -fdiagnostics-color -ffunction-sections 
-fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 
-DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT 
tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o
 -MF 
tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o.d
 -o 
tools/clang/tools/extra/clang-apply-replacements/CMakeFiles/obj.clangApplyReplacements.dir/lib/Tooling/ApplyReplacements.cpp.o
 -c 
/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  
  
/var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:171:35:
 error: no member named 'try_emplace' in 'std::map'
  
  auto InsertPos = Replaces.try_emplace(R, SourceTU);

We may close this temporarily and reopen it after the c++17 flag is ready.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131046/new/

https://reviews.llvm.org/D131046

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits