Skip to content

Conversation

jagerman
Copy link
Member

This simplifies the std::optional/std::experimental::optional macros by
having just one macro that provides the namespace (either std or
std::experimental), which removes some code duplication.

It also enables the std::optional test which was previously only
testing optional under c++14 mode with std::e::o, but skipping it under
c++17 mode with std::o.

@wjakob
Copy link
Member

wjakob commented Nov 15, 2016

Hmm, I think the argument for keeping both is that they may be different types in an actual implementation of libstdc++. So partial overloading just on std::optional may break std::experimental::optional code.

@jagerman
Copy link
Member Author

It still has both--they just get referenced via PYBIND11_OPTIONAL_NS::optional now instead of having two different chunks of code with the only difference being that one writes std::optional and the other std::experimental::optional.

@jagerman
Copy link
Member Author

(And we don't ever support both at once).

@wjakob
Copy link
Member

wjakob commented Nov 15, 2016

Yes -- but what if we're compiling code on a c++17 compilant compiler that was still using std::experimental::optional?

@wjakob
Copy link
Member

wjakob commented Nov 15, 2016

(I admit that I don't really get the policies about std::experimental namespace. For instance, will aliases there always exist or only temporarily?)

@wjakob
Copy link
Member

wjakob commented Nov 15, 2016

cc @aldanor

@aldanor
Copy link
Member

aldanor commented Nov 15, 2016

But I think that we do want to support both at once, that was one of the points. GCC has both and the experimental one is not going away in GCC 6. Maybe next major GCC or may be not. If you look at your compiler's experimental include folder, you'll find a bunch of stuff there that has become stable since.

@jagerman
Copy link
Member Author

jagerman commented Nov 15, 2016

Yes -- but what if we're compiling code on a c++17 compilant compiler that was still using std::experimental::optional?

At least for libstdc++, this is impossible: including <optional> in C++14 or below is an (intentional) compilation error, and including <experimental/optional> in C++17 is likewise a compilation error. (Edit: never mind, I'm clearly wrong about that.)

I took a look at clang with libc++, and, at least as of llvm 3.9, the current c++1z support doesn't have <optional>, so I can't really say what will happen there.

@jagerman
Copy link
Member Author

Hmm, perhaps I'm wrong about it, but I'm pretty sure it was the case with the gcc-7 snapshot when I submitted the other PR, but I can't reproduce that now. With the current snapshot, including both seems fine--I'll amend the PR.

And indeed they are not the same:

#include <experimental/optional>
#include <optional>
#include <iostream>

int main() {
    std::cout << "same: " << std::is_same<std::optional<int>, std::experimental::optional<int>>::value << std::endl;
}

outputs: same: 0

@jagerman
Copy link
Member Author

Never mind, I'm just misremembering the logic here; including both was always allowed. I'll fix the PR to just fix the test case.

@jagerman
Copy link
Member Author

jagerman commented Nov 15, 2016

My bad on that one (especially considering I wrote that logic in the first place), sorry for the noise.

I amended the PR to just add testing for std::optional when available.

@jagerman
Copy link
Member Author

And though we don't currently have any tests in C++17 mode, I tested it locally with a g++ 6.2 with -std=c++17 and it runs and passes both tests.

@wjakob wjakob merged commit 2e76daa into pybind:master Nov 15, 2016
@wjakob
Copy link
Member

wjakob commented Nov 15, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants