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

[Guillaume Papin]

  @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 :)


================
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