martinmoene / optional-lite

optional lite - A C++17-like optional, a nullable object for C++98, C++11 and later in a single-file header-only library
Boost Software License 1.0
403 stars 45 forks source link

Using `value_or()` changes stored value #60

Closed deadem closed 3 years ago

deadem commented 3 years ago

Code:

#include <https://raw.githubusercontent.com/martinmoene/optional-lite/master/include/nonstd/optional.hpp>
#include <iostream>
#include <string>

int main()
{
    nonstd::optional<std::string> s;
    s = "test";
    std::cout << (*s == "test") << std::endl;
    std::cout << (s.value_or("") == "test") << std::endl; // comment this line
    std::cout << (s->empty()) <<  std::endl;
}

Output: 1 1 1

Expected output: 1 1 0

If I comment line with value_or it works just fine. GCC version also works as expected, but clang and msvc does not.

Live code: https://godbolt.org/z/frqjfr

martinmoene commented 3 years ago

Thanks @deadem

I'm not able to reproduce msvc failing.

For various versions of clang, gcc, msvc:

Using -Doptional_CONFIG_SELECT_OPTIONAL=optional_OPTIONAL_NONSTD to use nonstd::optional under C++17 and later.

deadem commented 3 years ago

We're catch this error in msvc in live project, but I can't reproduce it in isolated environment... Still trying to reproduce. Maybe the clang fix fixes the msvc too?..

mortenfyhn commented 3 years ago

I can reproduce this. I've only seen it happen with std::string, not with for instance int.

With clang 6.0.0 and optional-lite 3.4.0:

#include "optional.hpp"
#include <cassert>
#include <string>

int main()
{
    nonstd::optional<std::string> x = "a";
    x.value_or("b");
    assert(!x->empty());
}

then

clang++ -std=c++11 main.cpp && ./a.out and clang++ -std=c++14 main.cpp && ./a.out

fails the assert, while

clang++ -std=c++17 main.cpp && ./a.out and clang++ -std=c++2a main.cpp && ./a.out

runs fine.


If I naively just delete the non-const value_or definition:

https://github.com/martinmoene/optional-lite/blob/c94a958c188ac0befea23184977e159eb8512ff4/include/nonstd/optional.hpp#L1454-L1458

then it runs fine building with the same clang and flags for C++11/14/17/20.

martinmoene commented 3 years ago

Thanks @mortenfyhn.

I had looked into it earlier, but failed to report back here. I have a suspicion it may be related to issue #61.

phprus commented 3 years ago

Clang also support ref-qualifiers from version 2.9. And MSVC from 19.0 (2015), and GCC from 4.8.1. (See: https://en.cppreference.com/w/cpp/compiler_support)

But optional_HAVE_REF_QUALIFIER defined only to gcc >= 4.9 and MSVC >= 19.1 (2017).

Replace line:

#define optional_CPP11_140_G490     ((optional_CPP11_OR_GREATER_ && optional_COMPILER_GNUC_VERSION >= 490) || (optional_COMPILER_MSVC_VER >= 1910))

to:

#define optional_CPP11_140_G490     ((optional_CPP11_OR_GREATER_ && (optional_COMPILER_CLANG_VERSION >= 290 || optional_COMPILER_GNUC_VERSION >= 490)) || (optional_COMPILER_MSVC_VER >= 1910))

and you get expected behavior.

I've tested on GCC 7 and Clang from Xcode 11 (Apple clang version 11.0.0 (clang-1100.0.33.17)).

Please, check this patch on msvc and other your compilers.

martinmoene commented 3 years ago

@deadem , think this issue is handled, #61 is not yet.