subject:"\[PATCH\] D25613\: \[clang\-move\] Don't overuse Replacements\:\:add."

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Haojian Wu via cfe-commits

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rL284236: [clang-move] Don't overuse Replacements::add. 
(authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D25613?vs=74677=74679#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D25613

Files:
  clang-tools-extra/trunk/clang-move/ClangMove.cpp

Index: clang-tools-extra/trunk/clang-move/ClangMove.cpp
===
--- clang-tools-extra/trunk/clang-move/ClangMove.cpp
+++ clang-tools-extra/trunk/clang-move/ClangMove.cpp
@@ -193,27 +193,6 @@
   return SourceText.str();
 }
 
-clang::tooling::Replacement
-getReplacementInChangedCode(const clang::tooling::Replacements ,
-const clang::tooling::Replacement ) {
-  unsigned Start = Replacements.getShiftedCodePosition(Replacement.getOffset());
-  unsigned End = Replacements.getShiftedCodePosition(Replacement.getOffset() +
- Replacement.getLength());
-  return clang::tooling::Replacement(Replacement.getFilePath(), Start,
- End - Start,
- Replacement.getReplacementText());
-}
-
-void addOrMergeReplacement(const clang::tooling::Replacement ,
-   clang::tooling::Replacements *Replacements) {
-  auto Err = Replacements->add(Replacement);
-  if (Err) {
-llvm::consumeError(std::move(Err));
-auto Replace = getReplacementInChangedCode(*Replacements, Replacement);
-*Replacements = Replacements->merge(clang::tooling::Replacements(Replace));
-  }
-}
-
 bool isInHeaderFile(const clang::SourceManager , const clang::Decl *D,
 llvm::StringRef OriginalRunningDirectory,
 llvm::StringRef OldHeader) {
@@ -251,32 +230,24 @@
const std::vector ,
llvm::StringRef FileName,
bool IsHeader = false) {
-  clang::tooling::Replacements InsertedReplacements;
+  std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
 std::replace(GuardName.begin(), GuardName.end(), '/', '_');
 std::replace(GuardName.begin(), GuardName.end(), '.', '_');
 std::replace(GuardName.begin(), GuardName.end(), '-', '_');
 
 GuardName = StringRef(GuardName).upper();
-std::string HeaderGuard = "#ifndef " + GuardName + "\n";
-HeaderGuard += "#define " + GuardName + "\n";
-clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
-   HeaderGuard);
-addOrMergeReplacement(HeaderGuardInclude, );
+NewCode += "#ifndef " + GuardName + "\n";
+NewCode += "#define " + GuardName + "\n";
   }
 
   // Add #Includes.
-  std::string AllIncludesString;
-  // FIXME: Add header guard.
   for (const auto  : Includes)
-AllIncludesString += Include;
+NewCode += Include;
 
-  if (!AllIncludesString.empty()) {
-clang::tooling::Replacement InsertInclude(FileName, 0, 0,
-  AllIncludesString + "\n");
-addOrMergeReplacement(InsertInclude, );
-  }
+  if (!Includes.empty())
+NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
@@ -299,36 +270,23 @@
 for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0;
  --RemainingSize, ++It) {
   assert(It < CurrentNamespaces.rend());
-  auto code = "} // namespace " + *It + "\n";
-  clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code);
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "} // namespace " + *It + "\n";
 }
 while (DeclIt != DeclNamespaces.end()) {
-  clang::tooling::Replacement InsertedReplacement(
-  FileName, 0, 0, "namespace " + *DeclIt + " {\n");
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "namespace " + *DeclIt + " {\n";
   ++DeclIt;
 }
-
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM));
-addOrMergeReplacement(InsertedReplacement, );
-
+NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM);
 CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
-  for (const auto  : CurrentNamespaces) {
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, "} // namespace " + NS + "\n");
-addOrMergeReplacement(InsertedReplacement, );
-  }
+  for (const auto  : CurrentNamespaces)
+NewCode += "} // namespace " + NS + "\n";
 
-  if (IsHeader) {
-clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
-   "#endif // " + GuardName + "\n");
-

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Eric Liu via cfe-commits

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

Awesome! Lg with one nit.




Comment at: clang-move/ClangMove.cpp:197
 clang::tooling::Replacement
 getReplacementInChangedCode(const clang::tooling::Replacements ,
 const clang::tooling::Replacement ) {

This is also dead I think :P


https://reviews.llvm.org/D25613



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

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Haojian Wu via cfe-commits

hokein updated this revision to Diff 74677.
hokein added a comment.

Delete one more function.


https://reviews.llvm.org/D25613

Files:
  clang-move/ClangMove.cpp

Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -193,27 +193,6 @@
   return SourceText.str();
 }
 
-clang::tooling::Replacement
-getReplacementInChangedCode(const clang::tooling::Replacements ,
-const clang::tooling::Replacement ) {
-  unsigned Start = Replacements.getShiftedCodePosition(Replacement.getOffset());
-  unsigned End = Replacements.getShiftedCodePosition(Replacement.getOffset() +
- Replacement.getLength());
-  return clang::tooling::Replacement(Replacement.getFilePath(), Start,
- End - Start,
- Replacement.getReplacementText());
-}
-
-void addOrMergeReplacement(const clang::tooling::Replacement ,
-   clang::tooling::Replacements *Replacements) {
-  auto Err = Replacements->add(Replacement);
-  if (Err) {
-llvm::consumeError(std::move(Err));
-auto Replace = getReplacementInChangedCode(*Replacements, Replacement);
-*Replacements = Replacements->merge(clang::tooling::Replacements(Replace));
-  }
-}
-
 bool isInHeaderFile(const clang::SourceManager , const clang::Decl *D,
 llvm::StringRef OriginalRunningDirectory,
 llvm::StringRef OldHeader) {
@@ -251,32 +230,24 @@
const std::vector ,
llvm::StringRef FileName,
bool IsHeader = false) {
-  clang::tooling::Replacements InsertedReplacements;
+  std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
 std::replace(GuardName.begin(), GuardName.end(), '/', '_');
 std::replace(GuardName.begin(), GuardName.end(), '.', '_');
 std::replace(GuardName.begin(), GuardName.end(), '-', '_');
 
 GuardName = StringRef(GuardName).upper();
-std::string HeaderGuard = "#ifndef " + GuardName + "\n";
-HeaderGuard += "#define " + GuardName + "\n";
-clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
-   HeaderGuard);
-addOrMergeReplacement(HeaderGuardInclude, );
+NewCode += "#ifndef " + GuardName + "\n";
+NewCode += "#define " + GuardName + "\n";
   }
 
   // Add #Includes.
-  std::string AllIncludesString;
-  // FIXME: Add header guard.
   for (const auto  : Includes)
-AllIncludesString += Include;
+NewCode += Include;
 
-  if (!AllIncludesString.empty()) {
-clang::tooling::Replacement InsertInclude(FileName, 0, 0,
-  AllIncludesString + "\n");
-addOrMergeReplacement(InsertInclude, );
-  }
+  if (!Includes.empty())
+NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
@@ -299,36 +270,23 @@
 for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0;
  --RemainingSize, ++It) {
   assert(It < CurrentNamespaces.rend());
-  auto code = "} // namespace " + *It + "\n";
-  clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code);
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "} // namespace " + *It + "\n";
 }
 while (DeclIt != DeclNamespaces.end()) {
-  clang::tooling::Replacement InsertedReplacement(
-  FileName, 0, 0, "namespace " + *DeclIt + " {\n");
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "namespace " + *DeclIt + " {\n";
   ++DeclIt;
 }
-
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM));
-addOrMergeReplacement(InsertedReplacement, );
-
+NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM);
 CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
-  for (const auto  : CurrentNamespaces) {
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, "} // namespace " + NS + "\n");
-addOrMergeReplacement(InsertedReplacement, );
-  }
+  for (const auto  : CurrentNamespaces)
+NewCode += "} // namespace " + NS + "\n";
 
-  if (IsHeader) {
-clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
-   "#endif // " + GuardName + "\n");
-addOrMergeReplacement(HeaderGuardEnd, );
-  }
-  return InsertedReplacements;
+  if (IsHeader)
+NewCode += "#endif // " + GuardName + "\n";
+  return clang::tooling::Replacements(
+  clang::tooling::Replacement(FileName, 0, 0, NewCode));
 }
 
 } // namespace
@@ -490,7 +448,11 @@
Range.getBegin(),

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Haojian Wu via cfe-commits

hokein updated this revision to Diff 74675.
hokein added a comment.

woohoo, delete more code!


https://reviews.llvm.org/D25613

Files:
  clang-move/ClangMove.cpp

Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -204,16 +204,6 @@
  Replacement.getReplacementText());
 }
 
-void addOrMergeReplacement(const clang::tooling::Replacement ,
-   clang::tooling::Replacements *Replacements) {
-  auto Err = Replacements->add(Replacement);
-  if (Err) {
-llvm::consumeError(std::move(Err));
-auto Replace = getReplacementInChangedCode(*Replacements, Replacement);
-*Replacements = Replacements->merge(clang::tooling::Replacements(Replace));
-  }
-}
-
 bool isInHeaderFile(const clang::SourceManager , const clang::Decl *D,
 llvm::StringRef OriginalRunningDirectory,
 llvm::StringRef OldHeader) {
@@ -251,32 +241,24 @@
const std::vector ,
llvm::StringRef FileName,
bool IsHeader = false) {
-  clang::tooling::Replacements InsertedReplacements;
+  std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
 std::replace(GuardName.begin(), GuardName.end(), '/', '_');
 std::replace(GuardName.begin(), GuardName.end(), '.', '_');
 std::replace(GuardName.begin(), GuardName.end(), '-', '_');
 
 GuardName = StringRef(GuardName).upper();
-std::string HeaderGuard = "#ifndef " + GuardName + "\n";
-HeaderGuard += "#define " + GuardName + "\n";
-clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
-   HeaderGuard);
-addOrMergeReplacement(HeaderGuardInclude, );
+NewCode += "#ifndef " + GuardName + "\n";
+NewCode += "#define " + GuardName + "\n";
   }
 
   // Add #Includes.
-  std::string AllIncludesString;
-  // FIXME: Add header guard.
   for (const auto  : Includes)
-AllIncludesString += Include;
+NewCode += Include;
 
-  if (!AllIncludesString.empty()) {
-clang::tooling::Replacement InsertInclude(FileName, 0, 0,
-  AllIncludesString + "\n");
-addOrMergeReplacement(InsertInclude, );
-  }
+  if (!Includes.empty())
+NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
@@ -299,36 +281,23 @@
 for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0;
  --RemainingSize, ++It) {
   assert(It < CurrentNamespaces.rend());
-  auto code = "} // namespace " + *It + "\n";
-  clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code);
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "} // namespace " + *It + "\n";
 }
 while (DeclIt != DeclNamespaces.end()) {
-  clang::tooling::Replacement InsertedReplacement(
-  FileName, 0, 0, "namespace " + *DeclIt + " {\n");
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "namespace " + *DeclIt + " {\n";
   ++DeclIt;
 }
-
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM));
-addOrMergeReplacement(InsertedReplacement, );
-
+NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM);
 CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
-  for (const auto  : CurrentNamespaces) {
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, "} // namespace " + NS + "\n");
-addOrMergeReplacement(InsertedReplacement, );
-  }
+  for (const auto  : CurrentNamespaces)
+NewCode += "} // namespace " + NS + "\n";
 
-  if (IsHeader) {
-clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
-   "#endif // " + GuardName + "\n");
-addOrMergeReplacement(HeaderGuardEnd, );
-  }
-  return InsertedReplacements;
+  if (IsHeader)
+NewCode += "#endif // " + GuardName + "\n";
+  return clang::tooling::Replacements(
+  clang::tooling::Replacement(FileName, 0, 0, NewCode));
 }
 
 } // namespace
@@ -490,7 +459,11 @@
Range.getBegin(), Range.getEnd()),
 "");
 std::string FilePath = RemoveReplacement.getFilePath().str();
-addOrMergeReplacement(RemoveReplacement, [FilePath]);
+auto Err = FileToReplacements[FilePath].add(RemoveReplacement);
+if (Err) {
+  llvm::errs() << llvm::toString(std::move(Err)) << "\n";
+  continue;
+}
 
 llvm::StringRef Code =
 SM.getBufferData(SM.getFileID(MovedDecl.Decl->getLocation()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Eric Liu via cfe-commits

ioeric added inline comments.



Comment at: clang-move/ClangMove.cpp:472
 std::string FilePath = RemoveReplacement.getFilePath().str();
 addOrMergeReplacement(RemoveReplacement, [FilePath]);
 

For deletions, you can simply use `add`, which now simply merges overlapping 
deletions. And I think it should be considered an error if `add` fails. With 
`add`, you can get rid of `addOrMergeReplacement`.


https://reviews.llvm.org/D25613



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

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

2016-10-14 Thread Haojian Wu via cfe-commits

hokein created this revision.
hokein added a reviewer: ioeric.
hokein added a subscriber: cfe-commits.

https://reviews.llvm.org/D25613

Files:
  clang-move/ClangMove.cpp


Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -251,32 +251,24 @@
const std::vector ,
llvm::StringRef FileName,
bool IsHeader = false) {
-  clang::tooling::Replacements InsertedReplacements;
+  std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
 std::replace(GuardName.begin(), GuardName.end(), '/', '_');
 std::replace(GuardName.begin(), GuardName.end(), '.', '_');
 std::replace(GuardName.begin(), GuardName.end(), '-', '_');
 
 GuardName = StringRef(GuardName).upper();
-std::string HeaderGuard = "#ifndef " + GuardName + "\n";
-HeaderGuard += "#define " + GuardName + "\n";
-clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
-   HeaderGuard);
-addOrMergeReplacement(HeaderGuardInclude, );
+NewCode += "#ifndef " + GuardName + "\n";
+NewCode += "#define " + GuardName + "\n";
   }
 
   // Add #Includes.
-  std::string AllIncludesString;
-  // FIXME: Add header guard.
   for (const auto  : Includes)
-AllIncludesString += Include;
+NewCode += Include;
 
-  if (!AllIncludesString.empty()) {
-clang::tooling::Replacement InsertInclude(FileName, 0, 0,
-  AllIncludesString + "\n");
-addOrMergeReplacement(InsertInclude, );
-  }
+  if (!Includes.empty())
+NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
@@ -299,36 +291,23 @@
 for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0;
  --RemainingSize, ++It) {
   assert(It < CurrentNamespaces.rend());
-  auto code = "} // namespace " + *It + "\n";
-  clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code);
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "} // namespace " + *It + "\n";
 }
 while (DeclIt != DeclNamespaces.end()) {
-  clang::tooling::Replacement InsertedReplacement(
-  FileName, 0, 0, "namespace " + *DeclIt + " {\n");
-  addOrMergeReplacement(InsertedReplacement, );
+  NewCode += "namespace " + *DeclIt + " {\n";
   ++DeclIt;
 }
-
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, 
MovedDecl.SM));
-addOrMergeReplacement(InsertedReplacement, );
-
+NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM);
 CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
-  for (const auto  : CurrentNamespaces) {
-clang::tooling::Replacement InsertedReplacement(
-FileName, 0, 0, "} // namespace " + NS + "\n");
-addOrMergeReplacement(InsertedReplacement, );
-  }
+  for (const auto  : CurrentNamespaces)
+NewCode += "} // namespace " + NS + "\n";
 
-  if (IsHeader) {
-clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0,
-   "#endif // " + GuardName + 
"\n");
-addOrMergeReplacement(HeaderGuardEnd, );
-  }
-  return InsertedReplacements;
+  if (IsHeader)
+NewCode += "#endif // " + GuardName + "\n";
+  return clang::tooling::Replacements(
+  clang::tooling::Replacement(FileName, 0, 0, NewCode));
 }
 
 } // namespace


Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -251,32 +251,24 @@
const std::vector ,
llvm::StringRef FileName,
bool IsHeader = false) {
-  clang::tooling::Replacements InsertedReplacements;
+  std::string NewCode;
   std::string GuardName(FileName);
   if (IsHeader) {
 std::replace(GuardName.begin(), GuardName.end(), '/', '_');
 std::replace(GuardName.begin(), GuardName.end(), '.', '_');
 std::replace(GuardName.begin(), GuardName.end(), '-', '_');
 
 GuardName = StringRef(GuardName).upper();
-std::string HeaderGuard = "#ifndef " + GuardName + "\n";
-HeaderGuard += "#define " + GuardName + "\n";
-clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0,
-   HeaderGuard);
-addOrMergeReplacement(HeaderGuardInclude, );
+NewCode += "#ifndef " + GuardName + "\n";
+NewCode += "#define " + GuardName + "\n";
   }
 
   // Add #Includes.
-  std::string AllIncludesString;
-  // FIXME: Add header guard.
   for (const auto  : Includes)
-AllIncludesString += Include;
+NewCode += Include;
 
-  if

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

[PATCH] D25613: [clang-move] Don't overuse Replacements::add.

6 matches

Site Navigation

Mail list logo

Footer information