Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around msvc bug #304

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Marc-Aldorasi-Imprivata

Outcome triggers a SFINAE bug in MSVC by attempting to default construct ErrorCondEnum in a type constraint. This patch works around it by using std::declval instead.

Reproduction source file (also on godbolt)

#include <outcome.hpp>

struct inner {
	explicit inner(int);
	inner(inner &&);
};
struct middle {
	inner inn;
	template <class... Args>
	explicit middle(Args &&...args) : inn(static_cast<Args &&>(args)...) {}
	middle(middle &&);
};
struct outer {
	middle mid;
	int i = 0;
};

namespace o = OUTCOME_V2_NAMESPACE;

o::result<outer> func(outer &&out) {
	return static_cast<outer &&>(out);
}

@BurningEnlightenment
Copy link
Collaborator

Thanks for the report! I've upvoted the DevCom ticket. However, I think we should hold back on merging the fix until the MSVC v17.12 release is imminent as the MSVC bug might be fixed in one of the next previews. You should mention in the DevCom ticket that this affects outcome (and therefore a boost library) and link to this pull request. This usually moves such tickets further up the priority list.

@ned14
Copy link
Owner

ned14 commented Sep 24, 2024

Also note we can't use declval from the STL, as it would drag in an include we don't currently bring in.

I believe there is a alternative declval somewhere around. In any case, don't be merging this until it gets fixed up.

@Marc-Aldorasi-Imprivata
Copy link
Author

basic_result already uses std::declval in the next constructor (line 457)

@ned14
Copy link
Owner

ned14 commented Sep 25, 2024

basic_result already uses std::declval in the next constructor (line 457)

Sigh. This is why #248 needs implementing.

Thanks for mentioning that, it must have slipped in at some point.

@BurningEnlightenment
Copy link
Collaborator

The MSVC devcom ticket has been marked as fixed, therefore I assume that this bug won't make it into a VS production release.

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

Successfully merging this pull request may close these issues.

3 participants