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

[Sean Silva]

================
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:
----------------
Edwin Vane wrote:
> 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?
OwningPtr is commonly used, but I'm not sure how well it plays with containers without move semantics. Not a big deal anyway.

================
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);
 
----------------
Edwin Vane wrote:
> 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.
But that's different because it expands to a comma separated list. This one can be just a (templated) function.

================
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;
+    }
+  }
----------------
Edwin Vane wrote:
> 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.
Please leave a FIXME then.

================
Comment at: cpp11-migrate/Transform.h:58-59
@@ +57,4 @@
+private:
+  bool ChangesMade;
+  bool ChangesNotMade;
+};
----------------
Edwin Vane wrote:
> 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.
Ok. The current names are really confusing, so at least leave a comment with the explanation you just gave.

Alternatively, just delete them since they aren't used anywhere.


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

BRANCH
  loop-convert

ARCANIST PROJECT
  clang-tools-extra