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

[Edwin Vane]

  Given the nature of the bug I'd say don't transform cases where the arg is already by-value and it has cv-qualifiers. We want the modernizer to not produce non-compiling code:)


================
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 += ' ';
+
----------------
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.

================
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)) {}
----------------
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.


http://llvm-reviews.chandlerc.com/D1600