[PATCH] clang-replace: Delete change description files

Stefanus Du Toit stefanus.du.toit at intel.com
Fri Aug 23 11:44:55 PDT 2013


  Specific comments down below.

  I really think this should be an option that's turned off by default! Removing files is fine if this is being done as part of some other tool (e.g. from cpp11-migrate) but for someone running this on the command line it could be quite a surprise...


================
Comment at: clang-replace/tool/ClangReplaceMain.cpp:77
@@ -50,3 +76,3 @@
   FileToReplacementsMap GroupedReplacements;
   if (!mergeAndDeduplicate(TUs, GroupedReplacements, SM))
     return 1;
----------------
Do you really want to delete the files if this failed?

================
Comment at: clang-replace/tool/ClangReplaceMain.cpp:80
@@ -53,3 +79,3 @@
 
   if (!applyReplacements(GroupedReplacements, SM))
     return 1;
----------------
And likewise for this. Seems to me they should only be removed if all of the operations succeeded. As a bonus, then you don't need ScopedFileRemover anymore!

================
Comment at: clang-replace/ApplyReplacements.h:38
@@ +37,3 @@
+/// \brief Collection of TranslationUnitReplacement files.
+typedef std::set<std::string> TUReplacementFiles;
+
----------------
Do you really need a set here? I assume that collectReplacementsFromDirectory will never encounter the same file twice, so you might as well just use a vector and avoid the overhead of using a set.


http://llvm-reviews.chandlerc.com/D1492



More information about the cfe-commits mailing list