- User Since
- Jun 25 2014, 4:17 PM (169 w, 4 d)
Fri, Sep 22
Please keep in mind that LLVM follows a weekly ping rate: https://llvm.org/docs/DeveloperPolicy.html#code-reviews .
This one passes for me too without the code change.
Thu, Sep 21
This makes sense.
Mon, Sep 18
It looks like this test passes prior to the code change in this patch. Can you please change the test?
Thu, Sep 14
Make methods private
Yeah, I accidentally committed a prior patch with the link to this diff. I will commit this patch today though so it should be updated.
Tue, Sep 12
Committed in r313025
Oops, I've put in the wrong diff link for r313025. This patch is still not committed.
Mon, Sep 11
You should be able to test it using a unittest if this is not exposed in the C API.
Fri, Sep 8
- rename namespace
- try to further decouple SourceSelectionArgument
Thu, Sep 7
LGTM. Do you have commit access?
Refactor the selection argument handling by using a SourceSelectionArgument interface with subclass for test selection and separate out the test code from the tool class.
Wed, Sep 6
Address review comments
Tue, Sep 5
- Always pass in RefactoringRuleContext to the refactoring rule's function.
- Remove now redundant overloads.
- Make LocalRename's actions members private.
- Add CommandLienParser FIXME comment.
- Verify that refactoring actions have unique command names.
- Add a comment that explains how a subcommand is selected.
- Test by checking the modified source directly. This allowed me to get rid of the refactoring result wrapper as well.
- Remove ASTRefactoringContext
- Address the other review comments
Mon, Sep 4
Please precommit the auto loop modernization though. You might want to use llvm::make_range for for-ranged loops as well.
Get rid of inheritance of RefactoringEngine by getting rid of the class itself.
The diagtool tests should go to test/Misc, and there's an existing diagtool tree test there that has to be updated as well.
LGTM with one inline comment below:
defs could work, but I think that we should stick with TableGen. As you've said, we'd be able to introduce some sort of structure instead of just code in the future which will be better (Ideally we'd try to do it now, but given the fact that the GSoC is done the current approach is acceptable). I would also like to see this analyzed and tested better in the future to ensure a complete coverage of AST, and I think the use of TableGen can potentially help with that.
Fri, Sep 1
TableGen is great for structured data, but we shouldn't use it just to embed code in it. What was the issue you've had with the build when the inc file was in include?
Rebase on top of trunk and https://reviews.llvm.org/D37210.
Rebase on ToT
Thu, Aug 31
- Add default implementation for handle()
- Merge two error handling functions into one
Wed, Aug 30
Rebase on top of https://reviews.llvm.org/D37291
Fixed a comment
Tue, Aug 29
Lgtm with two requests below
Mon, Aug 28
The fact TraverseCXXOperatorCallExpr can't call its super TraverseCXXOperatorCallExpr makes the current solution kind of broken. The super implementation of TraverseCXXOperatorCallExpr is responsible for dispatch to WalkUp##STMT functions which actually call VisitCXXOperatorCallExpr which our current visitor will never call because of the custom "override". I would like to suggest an alternative approach that I think solves the problem more "elegantly":
This works as well I think