Thanks Philip! Here are my comments * You should run clang/docs/tools/dump_ast_matchers.py to generate a new LibASTMatchersReference.html for the new matchers.
* In AddOverrideTransform.rst consider markup: ...adds the ``override`` virtual specifier... * The header comments all spell override with three r's. * Could you add a test that uses LLVM_OVERRIDE or some equivalent macro. Macro's are a matcher's worst nightmare... * Can you name 'Method' -> MethodId in AddOverrideMatchers.h. This is the naming scheme we're settling on. * In AddOverrideActions.cpp: o Assert that M != 0. See the other transform actions for the message we use for this case. o Add "FIXME" to the comment about the problem with comments and inline function bodies. From: Philip Dunstan [mailto:[email protected]] Sent: Wednesday, April 03, 2013 10:02 PM To: Dmitri Gribenko; [email protected]; Manuel Klimek; Vane, Edwin Subject: [PATCH] Re: Patches to add-override functionality to cpp11-migrate tool 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]<mailto:[email protected]> www.philipdunstan.com<http://www.philipdunstan.com> On Wed, Feb 13, 2013 at 11:32 PM, Philip Dunstan <[email protected]<mailto:[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]<mailto:[email protected]> www.philipdunstan.com<http://www.philipdunstan.com> On Wed, Feb 13, 2013 at 1:41 PM, Dmitri Gribenko <[email protected]<mailto:[email protected]>> wrote: On Fri, Feb 8, 2013 at 11:01 PM, Philip Dunstan <[email protected]<mailto:[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]<mailto:[email protected]>>*/
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
