[PATCH] Re: Patches to add-override functionality to cpp11-migrate tool

[Vane, Edwin]

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:phil at philipdunstan.com]
Sent: Wednesday, April 03, 2013 10:02 PM
To: Dmitri Gribenko; cfe-commits at cs.uiuc.edu; 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
phil at philipdunstan.com<mailto:phil at philipdunstan.com>
www.philipdunstan.com<http://www.philipdunstan.com>


On Wed, Feb 13, 2013 at 11:32 PM, Philip Dunstan <phil at philipdunstan.com<mailto:phil at philipdunstan.com>> 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
phil at philipdunstan.com<mailto:phil at philipdunstan.com>
www.philipdunstan.com<http://www.philipdunstan.com>

On Wed, Feb 13, 2013 at 1:41 PM, Dmitri Gribenko <gribozavr at gmail.com<mailto:gribozavr at gmail.com>> wrote:
On Fri, Feb 8, 2013 at 11:01 PM, Philip Dunstan <phil at philipdunstan.com<mailto:phil at philipdunstan.com>> 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 <gribozavr at gmail.com<mailto:gribozavr at gmail.com>>*/


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130404/c0a8a78e/attachment.html>