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
