> const Tree& build_tree(std::vector<SymbolInfo> info)
> {
>   std::vector<Tree> trees;
>
...
>
>   return trees[0];
> }

You are returning a reference to a local variable. When the function 
finishes, trees' destructor will be called and all of its contents will be 
deallocated. So you're returning a reference to invalid memory. If you're 
not worried about copying Trees around, you can just remove the & from the 
return type.

You might also want to assert( trees.size() > 0 ) to make sure the function 
worked properly and there's something there to return.

That's the only actual problem I see. Otherwise I'm a little worried, 
efficiency-wise, about how many vectors you're copying around, but that may 
not be worth worrying about at this stage.

 - Slime
 [ http://www.slimeland.com/ ]

Post a reply to this message

Slime wrote:
>> const Tree& build_tree(std::vector<SymbolInfo> info)
>> {
>>   std::vector<Tree> trees;
>>
> ...
>>   return trees[0];
>> }
> 
> You are returning a reference to a local variable.

DOH!! >_<

Yeah, that can't be good...

> You might also want to assert( trees.size() > 0 ) to make sure the function 
> worked properly and there's something there to return.

The loop right above that is while(trees.size() > 1). Each pass of the 
loop removes two trees and inserts one tree.

> That's the only actual problem I see. Otherwise I'm a little worried, 
> efficiency-wise, about how many vectors you're copying around, but that may 
> not be worth worrying about at this stage.

Yeah, that too...

-- 
http://blog.orphi.me.uk/
http://www.zazzle.com/MathematicalOrchid*

Post a reply to this message

>> You might also want to assert( trees.size() > 0 ) to make sure the 
>> function worked properly and there's something there to return.
>
> The loop right above that is while(trees.size() > 1). Each pass of the 
> loop removes two trees and inserts one tree.

It definitely matters less when you're the only one editing the code, but 
asserting things like this is still a good habit to get into. I noticed that 
the loop had that behavior, but what if:

 - The loop didn't work as intended and sometimes, for instance, broke out 
before inserting the new element?
 - The loop works right, but at a later date you change it for some 
unrelated reason and break it in a subtle way?
 - trees.size() is 0 in the first place? This could happen if an empty 
vector was passed in.

The alternative to adding an assert is a small risk that at some point in 
the future you'll spend hours tracking down a crash. The more asserts you 
use, the more time you'll save! It takes time to learn where to add them, 
and I admit I went for years without using a single one. Once you start to 
use them consistently, you can be surprised at how frequently you see them 
fail, and you learn more about your own potential to make mistakes.

By the way, I wouldn't have recommended adding that assert if the loop were 
while( trees.size() != 1 ), since that would make the assert perfectly 
redundant. Of course in that case, I would assert that trees.size() >= 1 
before the loop to make sure the loop could work properly.

 - Slime
 [ http://www.slimeland.com/ ]

Post a reply to this message

Orchid XP v8 <voi### [at] devnull> wrote:
>    Tree t = build_tree(symbols);

  In this case the error messages given by valgrind wouldn't have been
the clearest possible, but they nevertheless point to this line being
the ultimate culprit (which, ultimately, is caused by build_tree() returning
a reference to a local variable).

  Anyways, I'm wondering why you are passing the parameter by value rather
than by const reference...

-- 
                                                          - Warp

Post a reply to this message

On Sun, 05 Oct 2008 06:42:07 +0200, Chris Cason  
<del### [at] deletethistoopovrayorg> wrote:
>
> If he wants brevity he could also code it like this:
>
>   for (int i = 0; i < c.size(); i++)
>     out.append(c[i] ? "1" : "0");
>
> which is just as valid and avoids running the if/else together.

The compiler would likely warn about the signed/unsigned comparison in the  
loop condition. The proper type for the index is Codeword::size_type.

He could also skip the conditional in the loop body entirely:


typedef std::vector<bool> Codeword;

std::string codeword( Codeword const& c )
{
	std::string out;

	for( Codeword::size_type i = 0; i < c.size(); ++i )
		out += ( '0' + int(c[i]) );

	return out;
}



-- 
FE

Post a reply to this message

Fredrik Eriksson <fe79}--at--{yahoo}--dot--{com> wrote:
> std::string codeword( Codeword const& c )
> {
>         std::string out;

  Also after this it can be a good idea to do:

          out.reserve(c.size());

  This will make it slightly more efficient because it avoids reallocations
later, when the string is filled with that many characters.

-- 
                                                          - Warp

Post a reply to this message

Warp wrote:
> Orchid XP v8 <voi### [at] devnull> wrote:
>>    Tree t = build_tree(symbols);
> 
>   In this case the error messages given by valgrind wouldn't have been
> the clearest possible, but they nevertheless point to this line being
> the ultimate culprit (which, ultimately, is caused by build_tree() returning
> a reference to a local variable).

Yeah. I guess since the error only came to light when I added a new 
function, I assumed that the new function must be the problem. In fact 
it was just exhibiting a problem that already existed...

>   Anyways, I'm wondering why you are passing the parameter by value rather
> than by const reference...

Ooo, well spotted. That's simply an oversight on my part. Since all the 
syntax is still kinda new to me, I keep forgetting to write bits. (E.g., 
the compiler keeps complaining that I'm using signed integers to index 
things.)

-- 
http://blog.orphi.me.uk/
http://www.zazzle.com/MathematicalOrchid*

Post a reply to this message

Slime wrote:

> It definitely matters less when you're the only one editing the code, but 
> asserting things like this is still a good habit to get into.

OK, fair enough.

>  - trees.size() is 0 in the first place? This could happen if an empty 
> vector was passed in.

There probably *should* be an assert to check that the vector passed in 
is non-empty...

-- 
http://blog.orphi.me.uk/
http://www.zazzle.com/MathematicalOrchid*

Post a reply to this message

Chris Cason wrote:

> If he wants brevity he could also code it like this:
> 
>   for (int i = 0; i < c.size(); i++)
>     out.append(c[i] ? "1" : "0");
> 
> which is just as valid and avoids running the if/else together.

Ooo, I like that. (It makes it clear that append() is always called, and 
it's only the argument that changes depending on c[i].)

-- 
http://blog.orphi.me.uk/
http://www.zazzle.com/MathematicalOrchid*

Post a reply to this message

Slime wrote:

> If you're 
> not worried about copying Trees around, you can just remove the & from the 
> return type.

Changing this single character makes my program work as intended. Yay me!

I guess the efficient thing to do would be to have the caller allocate 
an empty tree and pass it in by reference and mutate that... Hmm, but 
how to be sure the tree passed in is empty? I guess I could just add 
another assert() for that...

-- 
http://blog.orphi.me.uk/
http://www.zazzle.com/MathematicalOrchid*

Post a reply to this message