STL: Scalar increment inside insert() - bad?

[Allan Stirrett]

Hi,

I've got a section of code that basically goes like so:

int l_Index = 0;
std::set<int> l_Set;
while(l_Index < 10)
{
l_Set.insert(l_Index++);
}

This USUALLY works okay (insert() doesn't appear to have
side-effects, so l_Index goes up by one each time), but at some point,
my app gets into a state where, by breaking into it with WinDBG, I can
see that the loop is repeating, but l_Index is not incrementing. I
know the insert() is working because I can step through it AND I can
see that memory is increasing.

QUESTION: Is this code not a good idea? If not, why not? The
easy solution is to move the increment OUT of the insert(), but I want
to be sure that it will solve the problem, since this state is
difficult to reproduce, and just because the new code SEEMS to work
doesn't mean I've solidly removed the problem.

Thanks in advance for any help on this one.

Allan Stirrett.

 

[Ivan Vecerina]

: Hi,
:
: I've got a section of code that basically goes like so:
:
: int l_Index = 0;
: std::set<int> l_Set;
: while(l_Index < 10)
: {
: l_Set.insert(l_Index++);
: }
This is totally valid code, as far as I can tell.
l_Index is a local variable, is not modified within the 'insert' call.
The post-increment will be applied anytime after the parameter to
insert() has been copied (may be before or after the actual call
of the function, but this won't matter here).

: This USUALLY works okay (insert() doesn't appear to have
: side-effects, so l_Index goes up by one each time), but at some point,
: my app gets into a state where, by breaking into it with WinDBG, I can
: see that the loop is repeating, but l_Index is not incrementing. I
: know the insert() is working because I can step through it AND I can
: see that memory is increasing.
Could it be that you were looking at an optimized build? What is then
observed in a debugger can then often be unexpected/confusing.

: QUESTION: Is this code not a good idea? If not, why not?
Well, there sure are better ways to write it...
For example:
std::set<int> l_Set;
for( int l_Index=0 ; l_Index<10 ; ++l_Index )
l_Set.insert(l_Index);

: The
: easy solution is to move the increment OUT of the insert(), but I want
: to be sure that it will solve the problem, since this state is
: difficult to reproduce, and just because the new code SEEMS to work
: doesn't mean I've solidly removed the problem.
There is no technical problem in the code fragment that you posted
(although its style/readability could be improved IMHO).

Regards,
Ivan

 

[Allan Stirrett]

: Hi,
:
: I've got a section of code that basically goes like so:
:
: int l_Index = 0;
: std::set<int> l_Set;
: while(l_Index < 10)
: {
: l_Set.insert(l_Index++);
: }
This is totally valid code, as far as I can tell.
l_Index is a local variable, is not modified within the 'insert' call.
The post-increment will be applied anytime after the parameter to
insert() has been copied (may be before or after the actual call
of the function, but this won't matter here).

Thank you for the affirmation. The code is so simple I'm starting to
get the feeling it's a debugger issue more than a coding problem.

: This USUALLY works okay (insert() doesn't appear to have
: side-effects, so l_Index goes up by one each time), but at some point,
: my app gets into a state where, by breaking into it with WinDBG, I can
: see that the loop is repeating, but l_Index is not incrementing. I
: know the insert() is working because I can step through it AND I can
: see that memory is increasing.
Could it be that you were looking at an optimized build? What is then
observed in a debugger can then often be unexpected/confusing.

That's always a possibility. I'll double-check that it is debug code
or at least release with symbols.

: QUESTION: Is this code not a good idea? If not, why not?
Well, there sure are better ways to write it...
For example:
std::set<int> l_Set;
for( int l_Index=0 ; l_Index<10 ; ++l_Index )
l_Set.insert(l_Index);

That is true. The example I gave is quite simplified: the l_Index is
actually set one of several ways in a large if() block, hence the
while() instead of for(). Alternatively, I could use:

... set l_Value at some point somehow ...
... set l_MaxIndex at some point somehow ...
for( ; l_Index < l_MaxIndex; ++l_Index )
l_Set.insert(l_Index);

though that would stir the whole "for() statement with missing parts"
debate!

Thank you for the input & suggestions.

Allan.

 

[Jorgen Grahn]

: Hi,
:
: I've got a section of code that basically goes like so:
:
: int l_Index = 0;
: std::set<int> l_Set;
: while(l_Index < 10)
: {
: l_Set.insert(l_Index++);
: } ....
There is no technical problem in the code fragment that you posted
(although its style/readability could be improved IMHO).

Agreed. There's nothing magic here, and set<T>::insert() isn't special in
any way. You evaluate the expression l_Index++ every time you pass through
the loop, simple as that.

Just because I'm curious: what are those annoying 'l_' prefixes for?
For me, they break the flow as I'm reading -- plus, the 'l' looks a lot like '1'.
I'd prefer this code (plus the switch to 'for' suggested by I.V.):

std::set<int> theset;

int i = 0;
while(i < 10) {
theset.insert(i++);
}

BR,
Jorgen

 

[Allan Stirrett]

Agreed. There's nothing magic here, and set<T>::insert() isn't special in
any way. You evaluate the expression l_Index++ every time you pass through
the loop, simple as that.

Indeed, checking back, the loop DID eventually exit after several hours
of execution (which should NOT have taken that long). However, my
variables never changed value, so it looks like WinDBG wasn't
displaying things correctly.

Just because I'm curious: what are those annoying 'l_' prefixes for?
For me, they break the flow as I'm reading -- plus, the 'l' looks a lot like '1'.
I'd prefer this code (plus the switch to 'for' suggested by I.V.):

std::set<int> theset;

int i = 0;
while(i < 10) {
theset.insert(i++);
}

It's a simplified Hungarian Notation we've adopted. Particularly in
large C++ methods, it helps to be able to see at a glance which are
local variables (l_XXX) versus class members (m_XXX) versus globals
(g_XXX). Once you get used to the look (it took me a while myself), it
DOES help. I hope this doesn't spark a war on HN ;-)

Allan.

 

[Jorgen Grahn]

....
It's a simplified Hungarian Notation we've adopted. Particularly in
large C++ methods, it helps to be able to see at a glance which are
local variables (l_XXX) versus class members (m_XXX) versus globals
(g_XXX). Once you get used to the look (it took me a while myself), it
DOES help.

Well, I need something on my class members myself -- m_foo or foo_. It would
take a lot to convince me that locals need l_foo -- or that I need globals
at all.

I hope this doesn't spark a war on HN ;-)

Agreed. We've all been through it a hundred times, after all.

/Jorgen

 

[Allan Stirrett]

Well, I need something on my class members myself -- m_foo or foo_. It would
take a lot to convince me that locals need l_foo -- or that I need globals
at all.

Just my two cents: if you're going to prefix some, might as well
prefix them all for consistency. At a glance, is "sequenceNum" a local
variable, or a member variable someone forgot to prefix with "m_"? If
members, locals, and globals (which I avoid at all costs BTW) are ALL
consistently prefixed, then there's no question about scope.

I doubt that is enough to convince you, but just my thoughts!