<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 5, 2013 at 2:44 PM, Guillaume Papin <span dir="ltr"><<a href="mailto:guillaume.papin@epitech.eu" target="_blank">guillaume.papin@epitech.eu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
  @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 :)<br></blockquote><div><br></div><div>
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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
<br>
================<br>
Comment at: test/clang-modernize/PassByValue/removing_const_fail.cpp:10<br>
@@ +9,3 @@<br>
</div><div class="im">+struct A {<br>
+  A(Movable const M) : M(M) {}<br>
+  // CHECK: A(Movable M) : M(std::move(M)) {}<br>
</div>----------------<br>
<div class="im">Edwin Vane wrote:<br>
> Guillaume Papin wrote:<br>
> > Edwin Vane wrote:<br>
> > > Why should this be transformed? It's not being passed by const reference.<br>
> > It's still a valid candidate for the move, it's a parameter used only once and copied in class local storage.<br>
> ><br>
> > 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).<br>

> ><br>
> > I can remove this for the current patch and fix the issue related to const as a part of my fixes for CM-141.<br>
> I'd leave this test case here. It's still related to the "already passed-by-value" nature of this fix.<br>
</div>The new code make this one unnecessary since only const-ref and non-const value are accepted now.<br>
There is already a test (see `struct P`) that tests const-value parameters.<br>
<div class="im"><br>
================<br>
Comment at: clang-modernize/PassByValue/PassByValueActions.cpp:143-146<br>
@@ -137,2 +142,6 @@<br>
</div><div class="im">     //  ~~~~~~~~~~~           ~~~^<br>
-    ValueStr += ' ';<br>
+    SourceLocation NextChar =<br>
+        Lexer::getLocForEndOfToken(ParamTL.getEndLoc(), 0, SM, LangOptions());<br>
+    if (isIdentifierHead(*FullSourceLoc(NextChar, SM).getCharacterData()))<br>
+      ValueStr += ' ';<br>
+<br>
</div>----------------<br>
<div class="im">Edwin Vane wrote:<br>
> Guillaume Papin wrote:<br>
> > Edwin Vane wrote:<br>
> > > Is all this extra work really necessary? Can't we rely on formatting to fix whitespace problems?<br>
> > 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 `&`).<br>

> ><br>
> > 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.<br>
> ><br>
> > 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.<br>

> 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.<br>
</div>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.<br>

<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1600" target="_blank">http://llvm-reviews.chandlerc.com/D1600</a><br>
</blockquote></div><br></div></div>