<br><br><div class="gmail_quote">On Mon, Feb 4, 2013 at 1:13 PM, Dmitri Gribenko <span dir="ltr"><<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Mon, Feb 4, 2013 at 3:32 AM, Philip Dunstan <<a href="mailto:phil@philipdunstan.com">phil@philipdunstan.com</a>> wrote:<br>
> Hi<br>
><br>
> These patches continue some work that I started back in July (before RL<br>
> kicked in) to create a C++11 migration tool that would add the override<br>
> specifier to suitable member functions where it could. I have reworked the<br>
> tool to integrate it into the cpp11-migrate tool in the tool/extra<br>
> repository.<br>
<br>
</div>Hi Philip,<br>
<br>
This looks like a great addition to the C++11 conversion tool!<br>
<div class="im"><br>
> There are two patches - one on the clang repo with some AST matchers and the<br>
> other in tool/extra. I have a couple of questions for each of the patches<br>
> that I would appreciate some help with.<br>
><br>
><br>
> add-override-clang_174296.patch<br>
> Adds ASTMatchers isCanonicalDecl, isVirtual and isOverride<br>
><br>
> Questions:<br>
> 1. How do I unit test the negative of isCanonicalDecl?<br>
> 2. Will someone help me with a better description of isCanonicalDecl for the<br>
> comment?<br>
<br>
</div>I'm unsure if isCanonicalDecl is what you want.<br>
<br>
methodDecl(hasParent(recordDecl())).bind("id") works the same way.<br>
<div class="im"><br>
> 1. I don't like the work in AddOverrideActions.cpp to identify the desired<br>
> location of the override keyword if there is an inlined body (work backwards<br>
> from the start of the body to the first non-whitespace character). This<br>
> doesn't work if there is a comment between the declaration and the body<br>
> (shown in the comment_before_inline_body_fail test) and I don't think it is<br>
> correct if there are user . Will someone please suggest a better way to do<br>
> this?<br>
<br>
</div>This might require improvements in Clang source code. But there's<br>
already a FunctionTypeLoc::getRParenLoc(), which might be what you<br>
want.<br>
<div class="im"><br>
> 2. I'm seeing a warning from the cpp11-migrate tool because C++11 is not<br>
> enabled in the compiler options during the initial parsing of the source. I<br>
> think that C++11 should be enabled as I expect this tool to be used on code<br>
> that has already been partly migrated to C++11. I would also expect that<br>
> this issue will arise as people run the migration tool with multiple<br>
> refactoring tool options.<br>
<br>
</div>I agree. Input file should compile as C++11 cleanly.<br>
<div class="im"><br>
> Should I be using the git repo or <a href="http://llvm-reviews.chandlerc.com" target="_blank">llvm-reviews.chandlerc.com</a> for these<br>
> changes?<br>
<br>
</div>You can do so, but you are not required to.<br>
<br>
+#include "llvm/Support/Compiler.h" // For LLVM_OVERRIDE<br>
<br>
In general, we don't write such comments.<br>
<br>
+ AddOverrideFixer(clang::tooling::Replacements &Replace,<br>
+ unsigned &AcceptedChanges,<br>
+ RiskLevel) :<br>
<br>
Please align 'clang' and 'unsigned'.<br>
<br>
If RiskLevel is not used now, why not drop it?<br>
<br>
+const char *Method = "method";<br>
<br>
I'm a bit worried about adding such definitions at TU scope. I know<br>
there are such definitions in other transformations already...<br>
<br>
+ return methodDecl(isCanonicalDecl(),<br>
+ isOverride(),<br>
+ unless(destructorDecl()),<br>
+ unless(hasOverrideAttr())<br>
+ ).bind(Method);<br>
<br>
').bind' looks weird. Please move to the previous line.<br>
<br>
+// Names to bind with matched expressions.<br>
+extern const char *Method;<br>
<br>
Three slashes?<br>
<br>
+int AddOverrideTransform::apply(const FileContentsByPath &InputStates,<br>
+ RiskLevel MaxRisk,<br>
<br>
Please align 'const' and 'RiskLevel'.<br>
<br>
+#include "clang/Lex/Lexer.h"<br>
+<br>
+<br>
+using namespace clang::ast_matchers;<br>
<br>
In general, we don't leave two consecutive empty lines. (There are<br>
multiple occurrences of this.)<br>
<br>
+void AddOverrideFixer::run(const<br>
ast_matchers::MatchFinder::MatchResult &Result) {<br>
<br>
80 cols. You can drop 'ast_matchers::'.<br>
<br>
+ const CXXMethodDecl* M = Result.Nodes.getDeclAs<CXXMethodDecl>(Method);<br>
<br>
Space should be adjacent to 'M'.<br>
<br>
+ // start at the beginning of the body and rewind back to the last<br>
<br>
s/start/Start/<br>
<br>
+ }<br>
+ else {<br>
<br>
This should be '} else {'.<br>
<br>
+AST_MATCHER(clang::CXXMethodDecl, isOverride) {<br>
+ return (Node.size_overridden_methods() > 0);<br>
+}<br>
<br>
I don't know if this is generally useful. You don't have to express<br>
everything as a matcher -- can filter out declarations in the<br>
callback.<br>
<span class="HOEnZb"><font color="#888888"><br>
Dmitri<br>
<br>
--<br>
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</font></span></blockquote></div><br>Hi Philip,<br><br>I seem to remember there was some magic in the [[clang::fallthrough]] attribute that allowed one to find the last macro that evaluated to this attribute (should one exist), and thus allowed the fix-it hints to use the macro spelling instead of the "pure" spelling.<br>
<br>Does your tool perform the same search ? Or do you just use the override keyword ?<br><br>-- Matthieu<br>