| Author |
 Topic  |
|
|
meissnersd
Senior Member
34 Posts |
 Posted - Jan 04 2007 : 11:59:06 AM
|
I am using version 1543
I have noticed a few bug with Refactor - Extract Method.
1) Extracted template methods sometimes have non-printable characters in the parameter lists if there is a syntax error in the code. It seem to stomp angle brackets.
In the below snippit count is missing.
On my machine, extracting the loop when count is commented out inserts nonprintable characters into parameter list of the extracted method
struct S
{
int v;
};
template< typename T >
void foo( std::vector<std::vector< S >> & twoD )
{
T i;
int j;
//int count; // <--- this is missing
std::vector<int> old(1000);
//-------------------extract---
for( i =0; i<(int)twoD.size(); ++i )
{
for( j =0; j<(int)twoD.size(); ++j )
{
old[count++] = twoD[i][j].v;
twoD[i][j].v = 0;
}
}
//-------------------extract---
}
2) The template parameters don't get correctly propagated.
In the below code, the extracted method needs to have
template<typename T>
keyword.
It is missing on my machine.
#include<stdio.h>
#include <vector>
struct S
{
int v;
};
template< typename T >
void foo( std::vector< std::vector< S > > & twoD )
{
T i;
int j;
//-------------------extract---
for( i =0; i<(int)twoD.size(); ++i )
{
for( j =0; j<(int)twoD.size(); ++j )
{
twoD[i][j].v = 0;
}
}
//-------------------extract---
}
3) Also the order of the arguments in the new extracted method is often..odd. It would be nice if the order was more predictable, for example first parameter arguments, then local variables, then members ...
|
|
|
feline
Whole Tomato Software
United Kingdom
18939 Posts |
Posted - Jan 04 2007 : 3:51:52 PM
|
I am seeing the same effect here. Thank you for the clear description. Some of this would have been quite hard to reproduce without this example 
Extract Method not producing a template function is:
case=4369
Extract Method using the "squares" rather than angle brackets is:
case=4370
The order of arguments, we are considering producing a dialog allowing you to rename and reorder the parameters for the new function, which will address point 3.
case=3462 |
zen is the art of being at one with the two'ness |
 |
|
|
meissnersd
Senior Member
34 Posts |
Posted - Jan 04 2007 : 8:00:19 PM
|
Sure!
I just wish all *my* bug reports came with steps to reproduce...
About the order..I guess I want it to default to a natural order...
I would suggest the declaration order of the extracted function
default to the order of declaration in the enclosing function.
For example
void src( A a, B b, C& c )
{
int i;
int j;
// extract ----------->
for( int i = 0; i<10; ++i)
{
++a;
++c;
}
// extract ----------->
}
}
to myfunc
void MyFunc( A& a, C& c, int& i )
{
for( int i = 0; i<10; ++i)
{
++a;
++c;
}
}
So A, B is skipped, then C, then i; j is skipped
Thanks!
Karl
|
|
|
|
meissnersd
Senior Member
34 Posts |
Posted - Jan 04 2007 : 8:06:57 PM
|
Also for bonus points...
Detect the case where you are working with class members.
Add a private member function to the cpp and h file and so we don't need arguments....
This might be too tricky....for example cases with multiple inheritance...macros...templates in the class declaration.
But I thought I would ask ;-)
|
|
|
|
schoenherr
Tomato Guru
Germany
160 Posts |
Posted - Jan 05 2007 : 01:58:57 AM
|
| for me using member vars works correctly |
|
|
|
feline
Whole Tomato Software
United Kingdom
18939 Posts |
Posted - Jan 05 2007 : 08:19:57 AM
|
quote:
Originally posted by meissnersd
Sure!
I just wish all *my* bug reports came with steps to reproduce...
you should see some of the bug reports that get posted here! it can take a while just to find out what programming language we are talking about 
as for Extract Method, it looks like the order of the parameters is the order in which they are referenced in the extracted function. knowing this the order that is produced makes a lot more sense.
I am wary of Extract Method adding class member variables, since people do not always want this. I know I try to find a sensible balance between parameters and member variables in my own code, and would not want VA just randomly stuffing extra member variables into my classes. |
zen is the art of being at one with the two'ness |
|
|
|
jpizzi
Tomato Guru
USA
642 Posts |
Posted - Jan 05 2007 : 10:50:40 PM
|
| meissnersd doesn't want any attributes added to the class. He/she wants Extract Method to detect that the variables are class attributes, so that it doesn't need to pass them in the parameter list. |
Joe Pizzi |
|
|
|
feline
Whole Tomato Software
United Kingdom
18939 Posts |
Posted - Jan 06 2007 : 8:08:57 PM
|
He is?
I have just run a simple test, and for me a class member variable is just referenced, and is not passed as a parameter. This is using VA 1543. |
zen is the art of being at one with the two'ness |
|
|
|
jpizzi
Tomato Guru
USA
642 Posts |
Posted - Jan 06 2007 : 11:25:31 PM
|
That's the way I read it. Time for him to pipe in....
|
Joe Pizzi |
|
|
|
meissnersd
Senior Member
34 Posts |
Posted - Jan 08 2007 : 8:24:42 PM
|
Hi Sorry for the delay. I had a busy weekend.
Ok Joe was correct, I was asking for extract to use classes instead of parameter list.
It appears this kind of works. I was not using it much day-to-day since it tends to break for me.
Here are a least two more bugs.
1) Extracted method does not correctly handle use after scope. In the below extracted method j should be pass by reference rather then by value.
namespace bar
{
class test2
{
protected:
int m2;
};
class test1 : public test2
{
int myMember;
public:
int doit();
};
int test1::doit() {
int i, j=0, k;
// start extract ---
for ( i = 0; i<10; ++i )
{
++j;
++myMember;
++m2;
}
// end extract ---->
k = j;
return 0;
}
}
void main(int argc, char** argv)
{
bar::test1 d;
d.doit();
}
2) Extract method does not correctly take into account class definition changes.
In the above example the two class definition was in foo.h and the methods were in foo.cpp.
I extracted the method. And it worked as above. Fine.
Then I did undo.
Then I cut and pasted the class definition from foo.h to foo.cpp. The h file was empty
Then I rebuilt all for the whole solution.
Then I extracted the method. It inserted the method into the h file. It had no class.
It incorrectly referenced the members. In that case they should have been passed in since it was not a member method.
Karl |
|
|
|
meissnersd
Senior Member
34 Posts |
Posted - Jan 08 2007 : 8:42:18 PM
|
and futher more...
Does not correctly handle extracted local variables.
extracted member variables should get left behind by the extraction.
Below the declaration of j gets moved to the new function...
int foo()
{
int i =0;
// start extract --
int j=100;
++i;
++j;
i += j;
// end extract --
return i + j;
}
ok I will stop now.
|
|
|
|
feline
Whole Tomato Software
United Kingdom
18939 Posts |
Posted - Jan 09 2007 : 10:40:45 AM
|
The first problem is actually caused by using pre-increment on the variables. Change it to post-increment and the problem goes away. That took a bit of discovering, so the code was very useful, since I could simply chop it down until I found the actual trigger for the problem.
case=4433
Problem 2, so far I cannot reproduce this. I am guessing that VA had simply not had time to reparse the files properly. By "Then I rebuilt all for the whole solution" I assume you mean you recompiled the solution. This does not help VA to reparse the files, and if anything will get in the way, since when the machine is busy compiling is not when you want VA to do lots of reparsing, potentially slowing down the rebuild.
Problem 3, Extract Method has done exactly what you asked it to do. This is actually covered by:
case=2063
which is described as "Extract Method should warn when local variables are referenced outside selection" |
zen is the art of being at one with the two'ness |
|
|
|
meissnersd
Senior Member
34 Posts |
Posted - Jan 09 2007 : 7:43:50 PM
|
| case=2063 yeah I guess a warning would make the most sense... |
|
|
|
feline
Whole Tomato Software
United Kingdom
18939 Posts |
Posted - Jan 10 2007 : 07:48:10 AM
|
A similar question came up in a different thread, about warnings from Extract Method. Currently Extract Method will let you remove "any" code, even if doing so is a bad idea.
Adding some basic checks to Extract Method probably makes sense, but given the complexity of C++ we also need people to think a little about what they are doing before running Extract Method, since asking VA to spot every potential problem is probably asking a bit much. |
zen is the art of being at one with the two'ness |
|
|
|
support
Whole Tomato Software
5566 Posts |
Posted - Jan 17 2007 : 3:45:30 PM
|
| Case 4433 is fixed in build 1544. |
|
|
| |
Topic |
|
Bug: refactor extract method - template functions
