Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.

huskier

The following for-loop code snippet runs normally:

for(auto item : m_image_disp_view->m_drawROI->getRoiRect()) {
    if(item.second.color == QColorConstants::Svg::magenta) {
        QRect rect = item.second.rect;
        mappedRect = transform.mapRect(rect);
        have_magenta_ROI = true;
        break;
    }
}

However, the following optimized code snippet using std::find_if compiles but crashes during runtime:

auto it = std::find_if(m_image_disp_view->m_drawROI->getRoiRect().begin(),
                       m_image_disp_view->m_drawROI->getRoiRect().end(),
                       [](const auto& item) {
                           return item.second.color == QColorConstants::Svg::magenta;
                       });

if (it != m_image_disp_view->m_drawROI->getRoiRect().end()) {
    QRect rect = it->second.rect;
    mappedRect = transform.mapRect(rect);
    have_magenta_ROI = true;
}

In the above code,

m_image_disp_view->m_drawROI->getRoiRect()
//is type of
std::map<int, ROI_INFO_STRUCT>

The definition of ROI_INFO_STRUCT is as follows:

typedef struct {
    QRect rect;
    QColor color;
} ROI_INFO_STRUCT;

The crash information is limited, we do not know how to dig this issue.

Pl45m4

@huskier said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

The crash information is limited, we do not know how to dig this issue.

And how should we know with even less information?
Debug and check where exactly it crashes.

return item.second.color == QColorConstants::Svg::magenta;

What does it return for your map? Does the iterator stop (= find item) within the map elements or does it go through and return the end of the map meaning nothing matching your specification was found?

Christian Ehrlicher

@huskier said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

m_image_disp_view->m_drawROI->getRoiRect()

This returns a temporary which gets destroyed immediately. The first loop only works by accident and will likely also crash in release mode.

And I don't see why std::find_if() should be faster here tbh.
Avoid the copy by using a const ref in the loop.

IgKh

@Christian-Ehrlicher said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

This returns a temporary which gets destroyed immediately. The first loop only works by accident and will likely also crash in release mode.

It is quite more subtle than that. The way that a range-based for loop is defined, the iteration target gets captured in a universal reference (either an lvalue or rvalue reference, as appropriate) scoped to the whole loop. If it is a temporary, the lifetime of that temporary is extended to live as long as the reference, but if it has references to other temporaries inside of it are those are not extended. A std::map<int, struct of value types> is most likely safe under most compilers and won't crash, but it is not certain. C++23 standardizes the specified behavior to require that the lifetime of all relevant temporaries must be extended until the loop ends.

For the std::find_if call none of that applies, and the begin and end iterators provided to it are 100% dangling pointers.

Not terribly relevant - the OP's code has other issues anyway, but thought it might be interesting for visitors who might not be familiar with that particular pitfall to know about.

huskier

@Pl45m4 Thanks for your reply.
For the std::find_if version, the crash error reads like:

@Christian-Ehrlicher Thanks for your help.
For the for-loop version, the code has changed into "const auto& item" instead of "auto item".

for(const auto& item : m_image_disp_view->m_drawROI->getRoiRect())

@IgKh Thanks for your explanation.
I think the code logic is quite simple. Maybe I am wrong.
Could you please suggest how should I optimize the code snippet? Can I use the std::find_if version in my case?
For now, the for-loop version works as expected.

IgKh

@huskier This has nothing to do about your logic, this has to do with C++ complexities.

Anyway, the correct way to write the std::find_if version is:

auto roiRect = m_image_disp_view->m_drawROI->getRoiRect();
auto it = std::find_if(roiRect.begin(), roiRect.end(),
                       [](const auto& item) {
                           return item.second.color == QColorConstants::Svg::magenta;
                       });

if (it != roiRect.end()) {
    QRect rect = it->second.rect;
    mappedRect = transform.mapRect(rect);
    have_magenta_ROI = true;
}

You have to ensure that the map lives until the end of the find_if call, and that all iterators come from the same map. In your original code, each call to getRoiRect gives you a new temporary map, so the begin and end iterators weren't pointing at the same map even - which is very explicitly spelled out for you in the crash message.

This version is exactly equivalent to:

auto roiRect = m_image_disp_view->m_drawROI->getRoiRect();
for (const auto& item : roiRect) {
    <snip>
}

The choice between the two is only a matter of style. Some prefer to use STL algorithms because they embody the "what" rather than the "how", some prefer straight loops due to their being more straightforward to follow.

huskier

@IgKh Thank you very much for the clear explanation and the example code!

jsulm

@huskier said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

the crash error reads like

As already suggested: use the debugger to see where in your code the crash happens.

SimonSchroeder

@huskier said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

For the std::find_if version, the crash error reads like:

This exactly says what @IgKh figured out: begin() and end() return iterators to different containers because you are working with temporaries. For most of us, this debug message would have made it a lot easier to spot this.

@jsulm said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

As already suggested: use the debugger to see where in your code the crash happens.

Which is also quite easy in this specific case: The error message already says to "Press Retry to debug the application". The buttons might be a little confusing at first. Just remember that "Retry" actually means "Debug". It can't get any easier than this to debug the application.

@huskier said in Optimizing the for-loop with std::find_if causes an unexpected crash during runtime.:

Can I use the std::find_if version in my case?

There is also std::ranges::find_if which will take a range instead of begin/end iterator pairs. At least you would be operating on a single temporary container. However, after the find_if you are creating a new temporary to get the end iterator. it will never match this end iterator even if it is the end iterator of the other temporary container (unless you are truly (un)lucky and the same memory address gets reused). Basically, the if is always true. I wouldn't be surprised if comparing iterators of two different containers is undefined behavior and the compiler is allowed to remove the if entirely (either only the condition or the whole block).

I'm also not sure if it->second is still valid. Lifetimes of temporaries are really complicated. You are on the safe side with the proposed solution to use a local variable instead of a temporary like @IgKh suggested (especially pre C++23).

huskier

Thank you very much for your guys.

Best regards.