@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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
