The only reason I didn't submit them through phabricator was my own ignorance.
I have resubmitted updated patches through phabricator as patch D629 and D630. These new patches also include all of the review changes requested by Manuel and Edwin, including * Separated the docs/ClangTools into the clang/tools/extra patch instead of the ASTMatchers patch. * Added missing puncuation in comments ASTMatchers.h * Removed unneeded paranthesis in return statement in ASTMatchers.h * Generated new LibASTMatchersReference.html * Added markup for override specifier in AddOverrideTransform.rst * Fixed misspelling of override in header comments * Added test that the transform works correctly if the override specifier is already specified in a macro. * Renamed Method to MethodId in AddOverrideMatchers.h * Added requested assert in AddOverrideActions.cpp * Added FIXME to comment in AddOverrideActions.cpp Phil -- Philip Dunstan [email protected] www.philipdunstan.com On Thu, Apr 4, 2013 at 11:57 AM, Manuel Klimek <[email protected]> wrote: > Review for: add-override.clang_178680.patch > > 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... > > On to the feedback: > > Overall looks good! Some minor nits: > > --- docs/ClangTools.rst (revision 178680) > +++ docs/ClangTools.rst (working copy) > This seems to be unrelated to the AST matcher changes and should be > coupled with the other patch I assume? > > +/// \brief Matches if the given method declaration is virtual > > Nit: here and below, we use punctuation to end sentences :) > > + return (Node.size_overridden_methods() > 0); > > Nit: remove unneeded parentheses. > > Cheers, > /Manuel > > > > On Thu, Apr 4, 2013 at 4:02 AM, Philip Dunstan <[email protected]>wrote: > >> 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. >> >> 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. >> >> Two patches are attached to this email: >> add-override.clang_178680.patch - Adds new AST matchers. >> add-override.clang_tools_extra_178680.patch - Adds the feature to the >> cpp11-migrate tool. >> >> Thanks. >> Phil >> -- >> Philip Dunstan >> [email protected] >> www.philipdunstan.com >> >> >> On Wed, Feb 13, 2013 at 11:32 PM, Philip Dunstan >> <[email protected]>wrote: >> >>> Thanks Dmitri and Manuel >>> >>> 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. >>> >>> 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. >>> >>> I'm happy to add the documentation when there is a somewhere to put it. >>> >>> Philip Dunstan >>> -- >>> Philip Dunstan >>> [email protected] >>> www.philipdunstan.com >>> >>> >>> On Wed, Feb 13, 2013 at 1:41 PM, Dmitri Gribenko <[email protected]>wrote: >>> >>>> On Fri, Feb 8, 2013 at 11:01 PM, Philip Dunstan <[email protected]> >>>> wrote: >>>> > Hello >>>> > >>>> > These patches continue some work that I started back in July (before >>>> RL >>>> > kicked in) to create a C++11 migration tool that would add the >>>> override >>>> > specifier to suitable member functions where it could. I have >>>> reworked the >>>> > tool to integrate it into the cpp11-migrate tool in the tool/extra >>>> > repository. >>>> >>>> +static bool isWhitespace(char C) { >>>> + return (C == ' ') || (C == '\t') || (C == '\f') || >>>> + (C == '\v') || (C == '\n') || (C == '\r'); >>>> +} >>>> >>>> You can use isWhitespace() from clang/Basic/CharInfo.h for this. >>>> >>>> + AddOverrideFixer(clang::tooling::Replacements &Replace, >>>> + unsigned &AcceptedChanges) : >>>> + Replace(Replace), >>>> + AcceptedChanges(AcceptedChanges) { } >>>> >>>> Please indent member initializers. >>>> >>>> This LGTM, but there's documentation missing -- obviously because >>>> there's no place to put it currently. After the patch to add a >>>> documentation page for cpp11-migrate lands, could you write up some >>>> user-visible documentation? >>>> >>>> Dmitri >>>> >>>> -- >>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if >>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/ >>>> >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
