Bug 196268

Created attachment 106683 [details]
function paramter dialog

Created attachment 106686 [details]
derived class declaration

Created attachment 106687 [details]
base class declaration

Created attachment 106688 [details]
base class implementation

Also, the refactor affected the usages of the refactored method. But it did so
by adding only a comma after the last (existing argument). For example, one
usage was refactored from:

    if (! children[i]->getMediaOffset(req, localSampleDesc, thisSuccess))

to

    if (! children[i]->getMediaOffset(req, localSampleDesc, thisSuccess,))

Product Version: NetBeans IDE Dev (Build 201102170501)
Java: 1.6.0_23; Java HotSpot(TM) Client VM 19.0-b09
System: Linux version 2.6.35.10-74.fc14.x86_64 running on i386; UTF-8; en_US (nb)
Userdir: /home/toddb/.netbeans/dev

Created attachment 106771 [details]
use case where I can not reproduce the issue

Todd,
Based on your screenshots I've created test case project, but I can't reproduce your issue.
Could you, please, modify it somehow, so I can reproduce the problem.

Thanks,
Vladimir.

(In reply to comment #5)
> Also, the refactor affected the usages of the refactored method. But it did so
> by adding only a comma after the last (existing argument). For example, one
> usage was refactored from:
> 
>     if (! children[i]->getMediaOffset(req, localSampleDesc, thisSuccess))
> 
> to
> 
>     if (! children[i]->getMediaOffset(req, localSampleDesc, thisSuccess,))
Just in case, what you've specified finally as default value?
I've realized that if I put space as default value => I will have issue like yours.

Btw, default value is the value which is inserted automatically in all function calls.
And I agree that it's useless to change signatures with /* = def */ style.

(In reply to comment #0)
> I noted that (perhaps a new feature) refactoring includes changing method
> arguments. I started with:
>     class TlTrackSegment {
>         ...
>         virtual bool getMediaOffset(const TlClipDesc&,
>                                     const MediaPlayReq& req,
>                                     MediaSampleDesc& sDesc,
>                                     bool& success);
> 
> and I wanted to change it to this:
>         virtual bool getMediaOffset(uint level,
>                                     const TlClipDesc&,
>                                     const MediaPlayReq& req,
>                                     MediaSampleDesc& sDesc,
>                                     bool& success);
> 
> The problems resulted as follows:
>  - The dialog insists on a default value for the added argument, which is not
> required for c++. This should be an optional part of the refactoring. See 1st
> attachment.
It is required, because specified value should be inserted in all existing function calls to prevent breaking of compilation.

>  - in derived classes it puts the default argument in /* */. This would be bad
> style. Changing the signature (optional vs not) in derived classes means that
> when using the derived class pointer, the optional argument is no longer
> optional. This should be an optional part of the refactoring. see second
> attachment. 
I will remove it unless check box "... only in Declarations" is checked.

>  - when I move the argument to 1st position it still generates an optional
> argument in the declaration. This is invalid c++.  See third attachment.
As above. Default means what to use in updated "calls", not in updated signatures. So it's needed, right?
>  - in the base class implementation, the default argument appears in /* */.
> Also very bad style, because that would suggest that the implementation can
> always expect the optional argument. This should be an optional part of the
> refactoring. See fourth attachment.
Will fix this part

Integrated into 'main-golden', will be available in build *201103140400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/79b8b6be8f0a
User: Vladimir Voskresensky <vv159170@netbeans.org>
Log: fixing #196268 -  refactoring method arguments produces bad code
-- use constants instead of numbers