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, Oct 16 '19 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, Oct 16 '19 at 22:26

David · Accepted Answer · 2019-10-16T21:18:33.273

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;

einpoklum · Answer 2 · 2019-10-17T06:26:47.777

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>";
}

C++ - Pointer to local variable within the function

2 Answers2