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

[Philip Dunstan]

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
phil at philipdunstan.com
www.philipdunstan.com


On Thu, Apr 4, 2013 at 11:57 AM, Manuel Klimek <klimek at google.com> 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 <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/20130405/03b12125/attachment.html>