C++ - Pointer to local variable within the function

Question

I know this can look like a rookie question already asked a thousand time. But I searched for the exact answer and I haven't found one...

I'm working on a code that, to sum up, fill an XML with different data.

I'm trying to optimize a part of it. The "naïve" code is the following:

xml << "<Node>";
for(auto& input : object.m_vec)
{
    if(input == "Something")
    {
        xml << input;
    }
}
xml << "</Node>";
for(auto& input : object.m_vec)
{
    if(input == "SomethingElse")
    {
        xml << "<OtherNode>";
        xml << input;
        xml << "</OtherNode>";
        break;
    }
}

The important thing is, while more than one input fit in <Node></Node>, only one fit in <OtherNode></OtherNode> (explaining the break;) and it may not exist either (explaining the xml << in-between the if statement).

I think I could optimize it such like:

std::vector<std::string>* VecPointer;

xml << "<Node>";
for(auto& input : object.m_vec)
{
    if(input == "Something")
    {
        xml << input;
    }
    else if(input == "SomethingElse")
    {
        VecPointer = &input;
    }
}
xml << "</Node>";

if(!VecPointer->empty())
{
    xml << "<OtherNode>"
        << *VecPointer
        << "</OtherNode>";
}

The point for me here is that there is no extra memory needed and no extra loop. But the pointer to the local variable bothers me. With my beginner's eyes I can't see a case where it can lead to something wrong.

Is this okay? Why? Do you see a better way to do it?

As it is right now, you risk using an uninitialized VecPointer in case you get no hits. That will crash the program. You could wrap it in some kind of smart pointer probably. Also, unless you're very careful that what it points to exists in the same scope you use the result, you could end up with other crashes. — csl
– csl, Commented Oct 16, 2019 at 20:52
Almost always, a pointer to a vector is a code smell. It isn't necessarily wrong, but it is rarely used or needed and should be generally avoided. This is because the vector itself, takes little room, typically 3 pointers because it creates it's objects on the heap and just points to them. So a pointer to pointers to the heap (where the objects are) is not efficient. Also harder to read. — doug
– doug, Commented Oct 16, 2019 at 22:26

David · Accepted Answer · 2019-10-16 21:18:33Z

1

You need to make sure your compairson also looks for an existing value within the VecPointer, since your original second loop only cares about the first value it comes across.

else if(VecPointer && input == "SomethingElse")

Don't look for ->empty(), as that's accessing the pointer and asking whether the pointed to vector is empty. If there's nothing to point to in the first place, you're going to have a bad time at the -> stage of the statement. Instead, if against it, since it's a pointer.

if(VecPointer)

Finally, you're using a Vector to save that one value from m_vec, which from other code I'm assuming is not a vector<vector<string>> but a vector<string> - in the latter case, your VecPointer should be std::string*

std::string* VecPointer = nullptr;

edited Oct 16, 2019 at 21:18

answered Oct 16, 2019 at 20:57

David

10.8k1 gold badge31 silver badges40 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

einpoklum · Accepted Answer · 2019-10-17 06:26:47Z

1

I'm trying to optimize a part of it.
...
Is this okay?

Maybe not! This may already be a poor use of your time. Are you sure that this is what's hurting your performance? Or that there's a performance problem at all?

Remember Don Knuth's old adage: Premature optimization is the root of all evil...

Do you see a better way to do it?

Consider profiling your program to see which parts actually take up the most time.

On an unrelated note, you could use standard library algorithms to simplify your (unoptimized) code. For example:

if (std::ranges::find(std::begin(object.m_vec) std::end(object.m_vec), "SomethingElse"s ) 
    != std::end(object.m_vec)) 
{
    xml << "<OtherNode>" << whatever << "</OtherNode>";
}

edited Oct 17, 2019 at 6:26

answered Oct 16, 2019 at 21:30

einpoklum

137k86 gold badges445 silver badges913 bronze badges

Collectives™ on Stack Overflow

C++ - Pointer to local variable within the function

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related