Possible dangling reference in ModifySafeMap::get about minetest HOT 7 CLOSED

Sorry, I didn't read closely and thought you meant the null_value.
it->second can never be a dangling reference since it's inside the map. You should test the other thing instead.

from minetest.

Actually, thinking about it again, the result is a prvalue (because of the lvalue-to-rvalue conversions), but we return an lvalue reference. So maybe there is a temporary object, that binds to the lvalue reference, resulting in a dangling reference.

On the other hand, 4) says, it's a glvalue:

4. If E2 and E3 are glvalues of the same type and the same value category, then the result has the same type and value category

This C++17 standard draft I found doesn't say at this point that it's a prvalue:

—(7.1) The second and third operands have the same type; the result is of that type and the result object is
initialized using the selected operand.

So, maybe we should read this as "the conversions are applied if necessary":

6. The lvalue-to-rvalue, array-to-pointer, and function-to-pointer conversions are applied to the second and third operands.

Anyway, we can just replace the ternary with an if.

from minetest.

I was unable to reproduce the warning on GCC nor do GCC Asan or Valgrind report any errors running the following unit test:

Have you treid a non-trivial type?
Or perhaps even one that prints in its con-/destructor?

from minetest.

void TestDataStructures::testMap2()
{
        ModifySafeMap<int, Tracker> map;
        TrackerState t0, t1;

        // overwrite
        map.put(1, Tracker(t0));
        map.put(1, Tracker(t1));
        std::cerr << "EVERYTHING PUT!\n";
        UASSERT(map.get(1));
        std::cerr << "GOT!\n";
        UASSERT(t0.deleted);
        UASSERT(!t1.copied);
        UASSERT(!t1.deleted);

        map.clear();
}

CONSTRUCTOR2
CONSTRUCTOR1
MOVE ASSIGNMENT
DESTRUCTOR
CONSTRUCTOR2
MOVE ASSIGNMENT
DESTRUCTOR
EVERYTHING PUT!
GOT!
DESTRUCTOR
[PASS] testMap2 - 2ms

I think it's a false positive.

from minetest.

Whoops, it may be necessary to test both.

void TestDataStructures::testMap2()
{
        ModifySafeMap<int, Tracker> map;
        TrackerState t0, t1;

        // overwrite
        map.put(1, Tracker(t0));
        map.put(1, Tracker(t1));
        std::cerr << "EVERYTHING PUT!\n";
        UASSERT(!map.get(-1));
        std::cerr << "GOT!\n";
        UASSERT(t0.deleted);
        UASSERT(!t1.copied);
        UASSERT(!t1.deleted);

        map.clear();
}

CONSTRUCTOR2
CONSTRUCTOR1
MOVE ASSIGNMENT
DESTRUCTOR
CONSTRUCTOR2
MOVE ASSIGNMENT
DESTRUCTOR
EVERYTHING PUT!
GOT!
DESTRUCTOR
[PASS] testMap2 - 0ms

It looks like the null value doesn't get converted to a temporary either.

from minetest.

according to these rules

Are referring to this point?

6.1) If both E2 and E3 now have the same type, the result is a prvalue of that type designating a temporary object(until C++17)whose result object is(since C++17) copy-initialized from whatever operand was selected after evaluating E1.

Since C++17, there's no temporary object. (Read it as: Result object of the prvalue will be copy-initialized from foo later, aka the prvalue points to foo. Not: The result of the expression is copy-initialized. (Easy to misread imo.))
(I didn't know there was a temporary object in pre 17. 👀)

from minetest.

Ah, phew, it seems safe in all cases since we're on C++17, then. I wonder if a bug report should be filed with MSVC?

(I'm not sure which one of the rules applies here, but that's probably the one. Even if I modify the code such that the result should be an l-value, which should not cause a temporary even pre-C++17, I still get the warning.)

from minetest.