Whole Tomato Software Forums
Whole Tomato Software Forums
Main Site | Profile | Register | Active Topics | Members | Search | FAQ
User name:
Password:
Save Password
Forgot your password?

 All Forums
 Visual Assist
 Technical Support
 Code Inspection - Loop refactoring on members
 New Topic  Reply to Topic
 Printer Friendly
Author Previous Topic Topic Next Topic  

khb
Tomato Guru

Germany
336 Posts

Posted - Mar 27 2017 :  11:32:41 AM  Show Profile  Visit khb's Homepage  Reply with Quote
Thank you for the code inspection. So far I love it ;)

One minor issue when refactoring loops (convert to range-based loop): If the container looped over is a member with a "m_" prefix, then please don't add it to the iterator variable. For example:
for (auto it = m_combinedResults.begin(); it != m_combinedResults.end(); it++)
is turned into:
for (auto & m_combinedResult : m_combinedResults)
but I would like to see:
for (auto & combinedResult : m_combinedResults)
Kind regards,
Marcus

feline
Whole Tomato Software

United Kingdom
15178 Posts

Posted - Mar 27 2017 :  12:56:40 PM  Show Profile  Reply with Quote
This is a fair point, I have put in a feature request for this:

case=104910

zen is the art of being at one with the two'ness
Go to Top of Page

sean
Whole Tomato Software

USA
2399 Posts

Posted - Apr 26 2017 :  5:07:28 PM  Show Profile  Visit sean's Homepage  Reply with Quote
case=104910 is fixed in build 2217.
Go to Top of Page

FlawedLogic
Junior Member

United Kingdom
23 Posts

Posted - Jul 12 2017 :  06:20:21 AM  Show Profile  Visit FlawedLogic's Homepage  Reply with Quote
For me I don't want you to change the iterator name at all. Not only does your alternative violate our coding standards (stack variables are lower case not capitalised), but changing the name can and does create confllicts with existing variables.

I don't know why you can't just keep the original name. Please make this an option at least.
Go to Top of Page

feline
Whole Tomato Software

United Kingdom
15178 Posts

Posted - Jul 12 2017 :  09:43:03 AM  Show Profile  Reply with Quote
Can you post an example of what you are seeing, both before and after? A simple example of where we are expecting this refactoring, and this change, to take effect, is the code:

class NamingTest
{
public:
    void test()
    {
        for (int i = 0; i < m_things.size(); ++i)
        {
            m_things[i] = 0; // creates iterator "thing"
        }
    }

    std::vector<int> m_things;
};

in these situations there is no existing iterator name to preserve, but removing the leading "m_" does make sense when creating an iterator name.

zen is the art of being at one with the two'ness
Go to Top of Page

FlawedLogic
Junior Member

United Kingdom
23 Posts

Posted - Jul 13 2017 :  03:10:34 AM  Show Profile  Visit FlawedLogic's Homepage  Reply with Quote
In that case there isn't. Actually I wasn't aware it even worked on indexed loops like that because it has never been offered in our code. The vast majority of our loops are iterating through containers. Typically the only use of an indexed loop in our code is when they do something, like break out prematurely, based on some value of the index in which case the refactor option is not offered so I haven't seen them.

I was thinking of the case given originally by khb, the iterator already has a name "it", so why not just preserve it?

Go to Top of Page

feline
Whole Tomato Software

United Kingdom
15178 Posts

Posted - Jul 13 2017 :  1:15:20 PM  Show Profile  Reply with Quote
That makes sense, and I have put in a feature request to preserve the current iterator name, when one exists:

case=109690

when the new name of the iterator already exists, does the duplicate name ever actually cause a compiler error, or is it just confusing? I have just been testing this here, and when the new iterator name would have clashed / been in the same scope as a variable with the same name, a different name was used. So I have not been able to generate an actual overlapping duplicate name, but it is certainly possible to get a confusing duplicate name.

zen is the art of being at one with the two'ness
Go to Top of Page

holedigger
Whole Tomato Software

145 Posts

Posted - Jul 13 2017 :  1:19:16 PM  Show Profile  Visit holedigger's Homepage  Reply with Quote
There are two variables/types here -- the iterator and sequence element. After the conversion the iterator variable doesn't exist anymore -- it is replaced by the element declaration. You are proposing we use the name of the iterator as the name of the element. This can cause confusion since the variable "iter" would no longer represents an iterator, the same way it would be strange to use "i" as the name when converting indexed loops.

Consider these examples:

// loop 1
for (auto it = m_combinedResults.begin(); it != m_combinedResults.end(); it++)
{
    auto& theResult = *it;
    theResult.Foo();
}

// loop 2
for (auto it = m_combinedResults.begin(); it != m_combinedResults.end(); it++)
{
    it->Foo();
}

In the first loop, the conversion reuses the name "theResult" since it was used to represent the current element, but in the second loop it has to invent a name.

Would you like it to prompt you for a name, or be able to rename it after the conversion?


Whole Tomato Software
Go to Top of Page

FlawedLogic
Junior Member

United Kingdom
23 Posts

Posted - Jul 14 2017 :  05:41:24 AM  Show Profile  Visit FlawedLogic's Homepage  Reply with Quote
I can see the problems you have, nothing is easy is it? My preference would be to have a global setting that either leaves the name exactly as it is, or changes it along your current rules. I wouldn't want to introduce yet another prompt for it.
Go to Top of Page
  Previous Topic Topic Next Topic  
 New Topic  Reply to Topic
 Printer Friendly
Jump To:
© 2017 Whole Tomato Software, Inc Go To Top Of Page
Snitz Forums 2000