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

[Manuel Klimek]

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 <phil at philipdunstan.com>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
> phil at philipdunstan.com
> www.philipdunstan.com
>
>
> On Wed, Feb 13, 2013 at 11:32 PM, Philip Dunstan <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
>> www.philipdunstan.com
>>
>>
>> On Wed, Feb 13, 2013 at 1:41 PM, Dmitri Gribenko <gribozavr at gmail.com>wrote:
>>
>>> On Fri, Feb 8, 2013 at 11:01 PM, Philip Dunstan <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>*/
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130404/e74611a6/attachment.html>