[cfe-commits] [PATCH] Port loop-convert into cpp11-migrate

[Sean Silva]

  Aside from the comments on the patch, just some food for thought:

  How do you anticipate this being used? The way I envision it is something like `git add -p`, where it interactively shows you diffs and you apply/reject them (perhaps with a command line option so that it skips interactively prompting you for changes above a given confidence level). In order to accommodate that I think Transform interface will need to be broadened (or some common API will need to be used by the transforms for applying changes).


================
Comment at: cpp11-migrate/Transform.h:58-59
@@ +57,4 @@
+private:
+  bool ChangesMade;
+  bool ChangesNotMade;
+};
----------------
Why are there two flags here? When would one not be the opposite of the other? Maybe a better name is necessary?

================
Comment at: cpp11-migrate/Transform.h:33
@@ +32,3 @@
+
+class Transform {
+public:
----------------
Doxygen documentation needed.

================
Comment at: cpp11-migrate/Transform.h:21-25
@@ +20,7 @@
+
+enum RiskLevel {
+  RL_Safe,
+  RL_Reasonable,
+  RL_Risky
+};
+
----------------
Doxygen.

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:39-42
@@ +38,6 @@
+// Helper RAII class for created Transforms.
+class TransformVec {
+public:
+  typedef std::vector<Transform*>::const_iterator const_iterator;
+
+public:
----------------
Can you just use a regular vector of smart pointers?

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:82-83
@@ -40,1 +81,4 @@
+
+#define AddTransformOpt(List, Name, T, Desc) \
+  List.getParser().addLiteralOption(Name, &ConstructTransform<T>, Desc);
 
----------------
Why is this a macro?

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:99-101
@@ +98,5 @@
+  TransformVec Transforms(TransformList.size());
+  for (unsigned int i = 0; i < TransformList.size(); ++i) {
+    Transforms.push_back(TransformList[i]());
+  }
+
----------------
Assign the end index to a variable.

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:114-115
@@ +113,4 @@
+  // Apply transforms.
+  for (TransformVec::const_iterator I = Transforms.begin();
+       I != Transforms.end(); ++I) {
+    if ((*I)->apply(MaxRiskLevel, OptionsParser.getCompilations(),
----------------
<http://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop>

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:123
@@ +122,3 @@
+  // Final Syntax check.
+  if (!TransformList.empty()) {
+    ClangTool EndSyntaxTool(OptionsParser.getCompilations(),
----------------
Not specifying any transforms should probably just be an error during option parsing.

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:124-129
@@ +123,8 @@
+  if (!TransformList.empty()) {
+    ClangTool EndSyntaxTool(OptionsParser.getCompilations(),
+                            OptionsParser.getSourcePathList());
+    if (EndSyntaxTool.run(
+        newFrontendActionFactory<clang::SyntaxOnlyAction>()) != 0) {
+      return 1;
+    }
+  }
----------------
What is the purpose of doing this if it doesn't roll back the changes?

If the tool "fails to transform my files" (`return 1`), then I expect it to not modify my files (or at least leave them in some well-defined state; or at the absolute minimum give me a useful diagnostic explaining the situation). The same could be said for the `return 1` in the transform-apply loop.


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

BRANCH
  loop-convert

ARCANIST PROJECT
  clang-tools-extra