[PATCH] Clang-Modernize.Pass-By-Value: Fix removing of namespace in parameter type

[David Blaikie]

On Thu, Sep 5, 2013 at 2:44 PM, Guillaume Papin
<guillaume.papin at epitech.eu>wrote:

>
>   @dblaikie: Since Edwin will be gone for some days and you have raised
> some good questions and shown some interests in this transform I allowed
> myself to add you to the reviewer list :)
>

I'll see what I can do, but ohterwise just ping the patch as usual
(~weekly) & as always, small easily understood patches are substantially
easier to review & more likely to get quick turnaround (this isn't a
judgment about this specific patch, just general advice - I haven't looked
at this one in detail yet).


>
>
> ================
> Comment at: test/clang-modernize/PassByValue/removing_const_fail.cpp:10
> @@ +9,3 @@
> +struct A {
> +  A(Movable const M) : M(M) {}
> +  // CHECK: A(Movable M) : M(std::move(M)) {}
> ----------------
> Edwin Vane wrote:
> > Guillaume Papin wrote:
> > > Edwin Vane wrote:
> > > > Why should this be transformed? It's not being passed by const
> reference.
> > > It's still a valid candidate for the move, it's a parameter used only
> once and copied in class local storage.
> > >
> > > I can skip this test the truth is this code will be changed. I can try
> to make this code invalid to a change but I think this solution will only
> make the code very-slightly more complex. TBH there are some other problem
> with const being on the right side (see CM-141).
> > >
> > > I can remove this for the current patch and fix the issue related to
> const as a part of my fixes for CM-141.
> > I'd leave this test case here. It's still related to the "already
> passed-by-value" nature of this fix.
> The new code make this one unnecessary since only const-ref and non-const
> value are accepted now.
> There is already a test (see `struct P`) that tests const-value parameters.
>
> ================
> Comment at: clang-modernize/PassByValue/PassByValueActions.cpp:143-146
> @@ -137,2 +142,6 @@
>      //  ~~~~~~~~~~~           ~~~^
> -    ValueStr += ' ';
> +    SourceLocation NextChar =
> +        Lexer::getLocForEndOfToken(ParamTL.getEndLoc(), 0, SM,
> LangOptions());
> +    if (isIdentifierHead(*FullSourceLoc(NextChar, SM).getCharacterData()))
> +      ValueStr += ' ';
> +
> ----------------
> Edwin Vane wrote:
> > Guillaume Papin wrote:
> > > Edwin Vane wrote:
> > > > Is all this extra work really necessary? Can't we rely on formatting
> to fix whitespace problems?
> > > I agree with you but the bug report complained about an extra space
> when it was not necessary, so I figured out this solution wasn't too
> complicated and may produce correct code without reformatting needed most
> of the time (for people using `A(const Foo & f);` (a space after `&`).
> > >
> > > The bug report complains about the case a param was already passed by
> value. I suppose I can handle this another way such as not replacing a
> parameter if it's already a value.
> > >
> > > Anyway, looking at the grammar I realized the check isn't quite
> correct, we can have a parameter with default value but no name for the
> parameter, in these cases a space before `=` is probably wanted so we
> better let the reformatting do the smart stuff.
> > I agree the modernizer shouldn't be concerned with the problems of
> whitespace except when it comes to producing correct code. Let formatting
> fix this.
> The new code now leaves value parameters untouched so the space problem
> described is 'fixed'. An extra-space will still be added if one was already
> present after the `&` but reformatting will take care of fixing this.
>
>
> http://llvm-reviews.chandlerc.com/D1600
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130905/9756cbbf/attachment.html>