<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:13px">Review for: add-override.clang_178680.patch</span><br><div><br></div><div style><font face="arial, sans-serif">First, I'd be curious to learn whether there's a reason for not using phab :) It's a lot more convenient for me to review patches that way, so I'd like to understand the reason and help to make any potential extra effort / concerns you have go away...</font></div>
<div style><font face="arial, sans-serif"><br></font></div><div style><font face="arial, sans-serif">On to the feedback:</font></div><div style><font face="arial, sans-serif"><br></font></div><div style><font face="arial, sans-serif">Overall looks good! Some minor nits:</font></div>
<div style><font face="arial, sans-serif"><br></font></div><div style><div>--- docs/ClangTools.rst (revision 178680)</div><div>+++ docs/ClangTools.rst (working copy)</div><div style>This seems to be unrelated to the AST matcher changes and should be coupled with the other patch I assume?</div>
<div style><br></div><div style><div>+/// \brief Matches if the given method declaration is virtual</div><div><br></div><div style>Nit: here and below, we use punctuation to end sentences :)</div><div style><br></div><div style>
<div>+ return (Node.size_overridden_methods() > 0);</div><div><br></div><div style>Nit: remove unneeded parentheses.</div><div style><br></div><div style>Cheers,</div><div style>/Manuel</div></div></div></div><div style>
<font face="arial, sans-serif"><br></font></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Apr 4, 2013 at 4:02 AM, Philip Dunstan <span dir="ltr"><<a href="mailto:phil@philipdunstan.com" target="_blank">phil@philipdunstan.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">These patches add an 'add-override' feature to the cpp11-migrate tool in clang/tool/extras. This feature adds the override keyword to virtual functions where applicable.<br>
<div><div class="gmail_extra">
<br></div><div class="gmail_extra">These patches were previously send to the list on Feb 13 but there appears to have been a problem with the sending of my previous email. I've rebased these patches so svn revision 178680 and added the information of the add-override feature to the cpp11-migrate tool documentation in the clang and clang tools extra repositories.<br>
<div class="gmail_extra"><br></div>Two patches are attached to this email:<br>add-override.clang_178680.patch - Adds new AST matchers.<br>add-override.clang_tools_extra_178680.patch - Adds the feature to the cpp11-migrate tool.<br>
<br></div><div class="gmail_extra">Thanks.<br></div><div class="gmail_extra">Phil<br>--<br><div>Philip Dunstan<br><a href="mailto:phil@philipdunstan.com" target="_blank">phil@philipdunstan.com</a><br>
<a href="http://www.philipdunstan.com" target="_blank">www.philipdunstan.com</a></div>
<br clear="all"></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 13, 2013 at 11:32 PM, Philip Dunstan <span dir="ltr"><<a href="mailto:phil@philipdunstan.com" target="_blank">phil@philipdunstan.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Thanks Dmitri and Manuel</div><div><br></div>I've attached a new patch to clang/tools/extra with Dmitri's suggestions to use isWhitespace() from clang/Basic/CharInfo.h and indent the member initializers of AddOverrideFixer.<div>
<br></div><div>For completeness, I have also re-attached the patch to the clang repository containing the additional matchers. This is unchanged except for it now being a diff against svn revision 175100.</div><div>
<br></div><div>I'm happy to add the documentation when there is a somewhere to put it.</div><span><font color="#888888"><div><br></div><div>Philip Dunstan</div></font></span><div class="gmail_extra"><div>
--<br><div>Philip Dunstan<br><a href="mailto:phil@philipdunstan.com" target="_blank">phil@philipdunstan.com</a><br>
<a href="http://www.philipdunstan.com" target="_blank">www.philipdunstan.com</a></div>
<br><br></div><div><div><div class="gmail_quote">On Wed, Feb 13, 2013 at 1:41 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>On Fri, Feb 8, 2013 at 11:01 PM, Philip Dunstan <<a href="mailto:phil@philipdunstan.com" target="_blank">phil@philipdunstan.com</a>> wrote:<br>
> Hello<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>+static bool isWhitespace(char C) {<br>
+ return (C == ' ') || (C == '\t') || (C == '\f') ||<br>
+ (C == '\v') || (C == '\n') || (C == '\r');<br>
+}<br>
<br>
You can use isWhitespace() from clang/Basic/CharInfo.h for this.<br>
<br>
+ AddOverrideFixer(clang::tooling::Replacements &Replace,<br>
+ unsigned &AcceptedChanges) :<br>
+ Replace(Replace),<br>
+ AcceptedChanges(AcceptedChanges) { }<br>
<br>
Please indent member initializers.<br>
<br>
This LGTM, but there's documentation missing -- obviously because<br>
there's no place to put it currently. After the patch to add a<br>
documentation page for cpp11-migrate lands, could you write up some<br>
user-visible documentation?<br>
<span><font color="#888888"><br>
Dmitri<span class="HOEnZb"><font color="#888888"><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" target="_blank">gribozavr@gmail.com</a>>*/<br>
</font></span></font></span></blockquote></div><span class="HOEnZb"><font color="#888888"><br></font></span></div></div></div></div>
</blockquote></div><br></div></div></div>
</blockquote></div><br></div>