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

[Edwin Vane]

  An interface as is presented with git add -p is just an idea for future work at this point. Right now, cpp11-migrate is going to apply transforms wholesale to files with no user input other than what is given on the command line. As mentioned in the design doc, there are plans to introduce more ways for user input especially as the transforms get more complicated. This is basically the very first commit for this tool. We have to start somewhere after all.


================
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:
----------------
Sean Silva wrote:
> Can you just use a regular vector of smart pointers?
I can do that. Does LLVM have a utility smart pointer that's recommended?

================
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);
 
----------------
Sean Silva wrote:
> Why is this a macro?
It mirrors the EnumValN macro from the command line library. The point is to make adding options for transforms nicer.

================
Comment at: cpp11-migrate/Cpp11Migrate.cpp:123
@@ +122,3 @@
+  // Final Syntax check.
+  if (!TransformList.empty()) {
+    ClangTool EndSyntaxTool(OptionsParser.getCompilations(),
----------------
Sean Silva wrote:
> Not specifying any transforms should probably just be an error during option parsing.
Just an arbitrary decision I made. I can make it at error if that's preferred.

================
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;
+    }
+  }
----------------
Sean Silva wrote:
> 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.
Reverting the state of files is something that's not fully implemented due to the simple interface provided by rewriting. The point of this commit is simply to port the loop convert tool into the cpp11-migrate tool to get things moving. Some work is going to need to be done with clang itself to get things to work properly according to the design.

================
Comment at: cpp11-migrate/Transform.h:58-59
@@ +57,4 @@
+private:
+  bool ChangesMade;
+  bool ChangesNotMade;
+};
----------------
Sean Silva wrote:
> Why are there two flags here? When would one not be the opposite of the other? Maybe a better name is necessary?
This is a port of loop-convert functionality. Loop-convert kept track of changes made, changes it might have made if they did not overlap with other changes (deferred it called them), and changes that it would have made if the user's tolerance for risk was high enough (rejected). Since I'm not sure what cpp11-migrate is going to do with regards to tracking that info yet I just hooked up the loop-convert functionality to these flags. 'changes made' indicates changes were actually made. 'changes-not-made' means there were changes not made (deferred or rejected by original nomenclature).

I didn't spend a lot of time thinking about nice names considering the uncertain nature of this functionality.


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

BRANCH
  loop-convert

ARCANIST PROJECT
  clang-tools-extra