![](/i/fill.gif) |
![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
> 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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
>> 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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Orchid XP v8 <voi### [at] dev null> 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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
On Sun, 05 Oct 2008 06:42:07 +0200, Chris Cason
<del### [at] deletethistoo povray org> 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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
Warp wrote:
> Orchid XP v8 <voi### [at] dev null> 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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
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
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |
| ![](/i/fill.gif) |
|
![](/i/fill.gif) |
|
![](/i/fill.gif) |
| ![](/i/fill.gif) |