================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
