C++ - Pointer to local variable within the function

user_10307096 0
Viewed 100

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?

pointers memory c++
2 Answers

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;
user_23911690

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