| Author |
 Topic  |
|
|
hotzenplotz
Senior Member
Austria
34 Posts |
 Posted - Sep 17 2007 : 6:21:38 PM
|
...and deletes comments:
// .h
namespace pirates
{
class arrr
{
public:
void bar(int x /*lalala*/, int y = 0); // <- change signature (make function const)
};
} // namespace pirates
namespace pirates
{
void arrr::bar(int /*x*/, /*comment234*/ int y) // test234
{
}
} // namespace pirates
results in this:
// .h
namespace pirates
{
class arrr
{
public:
void bar(int x /*lalala*/, int y = 0) const; // <- change signature (make function const)
};
} // namespace pirates
namespace pirates
{
void arrr::bar( int x /*lalala*/, int y /*= 0*/ ) const
{
}
} // namespace pirates
The .h file is fine, however the .cpp file is seriously messed up
1) The original comments are deleted.
I could live with that, especially if there where a warning message.
2) A comment is inserted for the default value.
Now that should *really* be configurable. I don't like "default value comments" in the implementation. Especially in new projects I sometimes change or remove default values for parameters. If I forget to do that via change signature, or if someone else who doesn't even use VAX does it, the comment in source file remains, stating a wrong default value.
Also I think it should be possible for VAX to figure out if a comment in the original code mirrors the old default value. If so, change it to reflect the new default value. If not, leave it untouched.
3) Additional spaces are inserted after the opening bracket for the paramteres and before the closing bracket.
This should be configurable too, or, even better, VAX should take the info from the original formatting.
4) Indentation is messed up.
As (3), VAX should keep the original indentation.
----
Also it would be great if one could reorder parameters, but if I try that, VAX doesn't change any references. And if I have to change references by hand, the feature isn't any good (for reordering parameters).
And without reordering parameters the feature is really only good for adding/removing qualifiers, and changing/syncing parameter names.
EDIT: that's not really a suggestion, just a description, right? So my suggestion is: fix it :)
|
Edited by - hotzenplotz on Sep 17 2007 6:24:23 PM |
|
|
sl@sh
Tomato Guru
Switzerland
204 Posts |
Posted - Sep 18 2007 : 05:56:29 AM
|
@Fixing references upon invocation of Change Signature: Your suggestion makes sense, but how would VAX discern a reordering of parameters from a simple change of types and parameter names? IME the most applications of Change Signature arise from adding or removing parameters or qualifiers, or change of parameter types. Any changes to parameters need to be fixed manually anyway, so adding the ability to automatically swap actual parameters under certain conditions wouldn't really add that much. In fact, it might confuse users because fixing the references in just a few cases would be a deviation from standard behaviour!
Moreover, users might disagree on what comprises a swapping of parameters as opposed to simple changes to parameter names and types.
@Comments for default parameters: IIRC that's been addressed elsewhere. I don't really care either way, so I leave the searching to you  |
 |
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Sep 18 2007 : 1:03:16 PM
|
Point 1, this is currently by design
Point 2, we are considering adding an option for this:
case=3495
Point 3, it is configurable, change the autotext entry "Refactor Create Implementation"
Point 4, this is actually very hard. Consider the implementation:
CFoo::DoSomething(int param1,
int param2, int param3
int param4
)
{
// ...
}
The user now removes any two parameters using Change Signature. What happens to the formatting?
What happens when the cpp and .h files have completely different multi-line formatting?
It is easy to dream up simple test cases where this is easy. Unfortunately there are also a lot of cases where this is not at all easy.
Reordering parameters, you can do this, and VA is designed to tell the difference between reordering parameters and renaming them:
http://www.wholetomato.com/products/features/changeSignature.asp
Updating all calls to the code, that is a different matter. If you only have simple calls then this is fine, but again it is quite easy to think up cases where this is less simple. As soon as you introduce default parameters into the equation then I don't see how VA can safely do so. |
zen is the art of being at one with the two'ness |
|
|
|
hotzenplotz
Senior Member
Austria
34 Posts |
Posted - Sep 18 2007 : 2:34:06 PM
|
> Point 1, this is currently by design
I thought so, since appearently the whole signature in the .cpp file is replaced.
> Point 2, we are considering adding an option for this:
That would be cool!
> Point 3, it is configurable, change the autotext entry "Refactor Create Implementation"
Ok, didn't know that. Thanks for the hint.
> Point 4, this is actually very hard. Consider the implementation: (...)
I figure you're using VS's "format selection" to format the newly created signature. So... is there any way to get VS to NOT indent the contents of namespaces?
|
|
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Sep 19 2007 : 12:17:11 PM
|
Point 4, there are probably some edge cases here, but changing the formatting of only one curly brace is not helpful, and seems to be covered by:
case=5758 |
zen is the art of being at one with the two'ness |
|
|
|
sean
Whole Tomato Software
USA
2817 Posts |
Posted - Jun 26 2014 : 01:54:28 AM
|
| case=3495 is implemented in build 2042 (see Refactoring page of VA Options dialog). |
|
|
|
Predelnik
Senior Member
Russia
37 Posts |
Posted - Aug 13 2014 : 09:26:55 AM
|
I probably just write here instead of creating new topic.
I think that changing signature for inherited methods becomes much more frustrating because it uncomments unused argument names. I think VAX should be able to detect it somehow and keep argument name commented at least in such case.
It's kinda like point 1 mentioned here but since in my case compilation depends on such comments I think it would be great if VAX could keep them. :( |
|
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Aug 14 2014 : 11:32:09 PM
|
Is your base class function an abstract virtual function?
If not, then we have to consider what happens when updating the signature. The argument names may be commented out in the declaration, but they cannot be used in the implementation if they are commented out there as well. |
zen is the art of being at one with the two'ness |
|
|
|
Predelnik
Senior Member
Russia
37 Posts |
Posted - Aug 15 2014 : 04:52:14 AM
|
I'm interested in a case when base class function isn't abstract but dummy one which doesn't use it's arguments, but it may as well be the one of the derived class virtual functions which does not uses all of the arguments.
And I'm talking about preserving commented arguments in implementation only, since it is one of the soft ways to avoid warning about unused arguments. I know that VAX will probably not work correctly with commented declaration arguments but I think it's ok mostly (though sometimes functions like mentioned are defined in headers and working correctly in that case would be great too but it's actually not that bad, afterall I can just move such functions to cpp). |
|
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Aug 15 2014 : 7:51:47 PM
|
Here is a very simple code sample to start looking at this, so we can actually discuss something specific. These functions are not implemented, so we only have the declarations to think about for now:
class testFelineSimpleClass
{
public:
virtual void testChangeSigComments(int nParam1/*, int nParam2*/);
};
class testFelineChildClass : public testFelineSimpleClass
{
virtual void testChangeSigComments(int nParam1/*, int nParam2*/);
};
Triggering Change Signature on this function, and updating the signature to:
virtual void testChangeSigComments(int nParam1, int nParam3/*, int nParam2*/);
works correctly, both functions are updated, and the comments are preserved. How does this compare to what you are doing and seeing? |
zen is the art of being at one with the two'ness |
|
|
|
Predelnik
Senior Member
Russia
37 Posts |
Posted - Aug 18 2014 : 05:17:24 AM
|
Sorry for the late answer and maybe misleading talk about inheritance, here's a simple example:
class testClass
{
void testFunc (int nParam1, int nParam2);
void doTheThing () {};
};
void testClass::testFunc (int nParam1, int /*nParam2*/)
{
if (nParam1) // nParam1 is used
doTheThing ();
// but nParam2 is unused so if it's uncommented I get a horrible warning which is treated as error
}
If I try to change signature of testFunc and rename second param to nParam3 it becomes uncommented in implementation.
I actually mentioned inheritance just because unused arguments are often happening in that case: like I want the base class implementation to do nothing so class wouldn't have to override this function to be non-abstract.
But thinking about it more I guess it's kinda bad to modify user comments. On the other hand I would certainly be more happy even if comment would stay unchanged.
And also I think that it could be implemented this way: if implementation didn't have argument name mentioned before or it was commented out then leave it that way and write argument name in comment (the latter maybe optional). Though maybe such thing would be hard to track I'm not sure.
Anyway, looks like no one bothers about stuff like this so maybe that's not that important, sorry for writing about this too much)) |
Edited by - Predelnik on Aug 18 2014 05:18:16 AM |
|
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Aug 18 2014 : 11:49:39 AM
|
The first problem here is that the function is different in the declaration and the implementation. So if you make a change, how do we know how to apply that change? We have taken the approach of making both the same, since we have to do something when the declaration and implementation are different.
You have told me the comment is the parameter name, but it's a comment, it could be anything at all. We don't really want to get into parsing comments as if they were code, just to see if they make sense as code.
Looking at arguments without a name, how do we know what you are doing when updating a function like this? Are you adding an extra parameter? Giving a parameter with no name a name? We watch the parameter names to help us work out what change you made to the signature. This works fairly well, but does assume there are names to track.
In general, I am confused why you have a virtual function with a commented out argument. If the argument is not used in the base class, make the function abstract. If the base class function actually uses the argument, then it won't be unused. If only the derived class version of the function wants and needs an extra parameter, maybe the function is not the same? |
zen is the art of being at one with the two'ness |
|
|
|
Predelnik
Senior Member
Russia
37 Posts |
Posted - Aug 18 2014 : 1:33:36 PM
|
Well if that's the case then I guess I would have too use this function very carefully since that's the approach in codebase I work with. I actually thought that VAX may distinguish changes like renaming argument or changing it's type and work separately in each case, but I guess if keeping signatures in declaration and implementation the same is the way then there's not much could be done for functions implemented in such manner.
About when it's needed: I guess it's easy to give real life example, let's say that I have an abstract class which is responsible for handling typical mouse events like move, click, press, release, drag etc. It has a lot of descendants which are handling these events in different situations or for different objects. Some of these child classes need like only one or two events to be handled out of 5, some need all of them. And I think that writing dummy methods for every descendant class which uses only one event out of 5 is pretty strange (but there would be some unused parameters either way) so basically making top hierarchy class not abstract but just having dummy functions implemented is pretty viable approach in my opinion. The same example could also be used to show that in some functions some arguments will be unused, like if in some cases it doesn't matter which button was used for mouse click or mouse coordinates were not of any matter. IMHO it's not such a strange pattern to use.
But I guess not that many people are using WError to bother or use different approach like FIX_UNUSED macro. Sorry for the trouble :) |
Edited by - Predelnik on Aug 18 2014 1:35:58 PM |
|
|
|
feline
Whole Tomato Software
United Kingdom
19024 Posts |
Posted - Aug 18 2014 : 4:06:44 PM
|
For Change Signature, consider this case. The before function is:
class testClass
{
void testFunc (int nParam1, int);
};
Now run Change Signature and tell VA that the new signature is:
void testFunc (int nParam1, int, int);
What happened to the second argument? Did it now become the 3rd argument? Or is the 2nd argument still the second argument and the 3rd argument is the new argument? This matters when updating the implementation, where the arguments are named.
Because we are letting you just edit the signature, edge cases like this are possible.
A different problem, the declaration says:
class testClass
{
void testFunc (int nParam1, int nParam2);
};
and the implementation says:
void testClass::testFunc (int nParam2, int nParam1)
which one do we believe? Reconciling different declarations and implementations is tricky sometimes.
Thank you for the explanation, in a situation like that I would think in terms of default parameters, and then the first thing the function implementation does is check to see if the default parameters just have the default / "ignore me" value. Then you can have as close to one function as possible, ignoring the extra parameters most of the time. I am not sure if that would work well for you, but it might be worth considering going forward.
No trouble, I am here to help, and its always better to ask if you are not sure about something  |
zen is the art of being at one with the two'ness |
|
|
| |
Topic |
|
|
|
Change signature messes up formatting...
