ned14 / outcome Goto Github PK

This self-contained example failed to compile (MSVC14)

#include <boost/monad/future.hpp>

int main()
{
    boost::monad::lightweight_futures::future<void> f;
    f.get();
    return 0;
}

If you change void to int, it compiles w/o problem. Is future<void> not supported?

I am trying to compile Boost.Outcome on MinGW distro: a platform for running GCC (6.3.0) on Windows (7). Here are details of my compiler. I think it is "new enough".

Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=c:/_data/cpp/mingw/bin/../libexec/gcc/x86_64-w64-mingw32/6.3.0/lto-wrapper.exe
Target: x86_64-w64-mingw32
Configured with: ../src/configure --enable-languages=c,c++ --build=x86_64-w64-mingw32 --host=x86_64-w64-mingw32 --target=x86_64-w64-mingw32 --disable-multilib --prefix=/c/temp/gcc/dest --with-sysroot=/c/temp/gcc/dest --disable-libstdcxx-pch --disable-nls --disable-shared --disable-win32-registry --enable-checking=release --with-tune=haswell
Thread model: win32
gcc version 6.3.0 (GCC)

When I try to compile this program:

#include <boost/outcome.hpp>

int main() {}

g++ -std=c++14 -IC:\_data\cpp\boost.outcome-master\include  -Wall test.cpp -o test.exe
In file included from C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome.hpp:26:0,
                 from C:\_data\cpp\boost.outcome-master\include/boost/outcome.hpp:1,
                 from test.cpp:1:
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp: In function 'char** backtrace_symbols(void* const*, size_t)':
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1356:34: error: 'memset' was not declared in this scope
       memset(&ihl, 0, sizeof(ihl));
                                  ^
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1409:52: error: '_ui64toa_s' was not declared in this scope
           _ui64toa_s((size_t) bt[n], p, end - p, 16);
                                                    ^
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1410:26: error: 'strchr' was not declared in this scope
           p = strchr(p, 0);
                          ^
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1418:47: error: '_itoa_s' was not declared in this scope
         _itoa_s(ihl.LineNumber, p, end - p, 10);
                                               ^
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1419:24: error: 'strchr' was not declared in this scope
         p = strchr(p, 0) + 1;
                        ^
make: *** [Makefile:2: main] Error 1

When I compile this program (with manually added headers):

#include <string.h>
#include <stdlib.h>
#include <boost/outcome.hpp>

int main() {}

I get this compiler output:

g++ -std=c++14 -IC:\_data\cpp\boost.outcome-master\include  -Wall test.cpp -o test.exe
In file included from C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome.hpp:26:0,
                 from C:\_data\cpp\boost.outcome-master\include/boost/outcome.hpp:1,
                 from test.cpp:3:
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp: In function 'char** backtrace_symbols(void* const*, size_t)':
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1409:52: error: '_ui64toa_s' was not declared in this scope
           _ui64toa_s((size_t) bt[n], p, end - p, 16);
                                                    ^
C:\_data\cpp\boost.outcome-master\include/boost/outcome/outcome_v1.0.hpp:1418:47: error: '_itoa_s' was not declared in this scope
         _itoa_s(ihl.LineNumber, p, end - p, 10);
                                               ^
make: *** [Makefile:2: main] Error 1

Is it me doing something wrong, or is it a bug?

Here's an inconsistency between the std and your version.

#include <cstdlib>
#include <iostream>
#include <future>
#include <boost/outcome/future.hpp>

namespace lw = boost::outcome::lightweight_futures;

int main(int argc, char *argv[])
{
    try
    {
        lw::promise<int> p;
        //std::promise<int> p;
        p.get_future();
        p.set_value(1);
        std::cout << "done\n";
    }
    catch (std::exception& e)
    {
        std::cout << e.what() << "\n";
    }

    return EXIT_SUCCESS;
}

The std version prints done, but the lw version throws an exception no state.

Is the inconsistency by design or is it a bug?

When fixing issue #12 last night, I undid a hack which had trivial copy/move constructors and assignment used when the type is trivially destructible. This allowed basic_monad to work in constexpr with types with trivial copy/move constructors and assignment, but it failed if the trivial types had non-trivial copy/move constructors which caused issue #12.

Right now value_storage uses one of four storage implementations:

• trivial destructors of T, EC and E mean use value_storage_impl_trivial
• otherwise use value_storage_impl_nontrivial
• additionally there is a bool template parameter selecting packed storage or not.

Instead we probably need a hierarchy of storage implementations:

• trivial destructibility means we make clear() do nothing, no destructors need be invoked during state changes
• trivial copy constructor needs to be detected (it's much more likely than a trivial move constructor) and we must not use placement new in that situation, indeed the default copy/move constructor is probably fine
• packed storage should be dropped I think as GCC 7 made constexpr bitfields illegal rather than implementing them like clang or MSVC.

Under this new schema you would get:

• value_storage_impl_tc_td - trivially copyable, trivially destructible
• value_storage_impl_ntc_td - non-trivially copyable, trivially destructible
• value_storage_impl_tc_ntd - trivially copyable, non-trivially destructible (***)
• value_storage_impl_ntc_ntd - non-trivially copyable, non-trivially destructible

Issues outstanding:

• (***) is trivially copyable, non-trivially destructible actually possible for a variant?
• should we really ignore move construction completely if copy construction is trivial?
• should we not mark anything calling placement new with constexpr so the compiler trips correctly? Thing is, all of the major compilers do let you call placement new in constexpr sometimes even though the standard says they should not. There is no good technical reason to ban it. I can foresee a standards change eventually.
• we didn't mention trivial assignment above at all. Some stupid types might have non-trivial copy constructors yet somehow a trivial copy assignment, or more likely vice versa. Handling this would double the count above though.

Add in check() functions for internal usage

http://ned14.github.io/boost.outcome/md_doc_md_03-tutorial_b.html#try_operation

it says

Unlike the try operator, the macro is not an expression so you can't write statements like if(BOOST_OUTCOME_TRY(var, expr)) as you can in Rust or Swift, this is due to a limitation of the C++ language (you can't return from functions in an expression).

However, I found this implementation of the TRY macro that seems to get around that issue: https://github.com/oktal/result

Need a unit test case exercising the Args&&...

Suggestions made:

• basic_monad -> outcome_base or basic_outcome
• monad_error -> bad_outcome or outcome_error

Two votes were for bad_outcome, I do like the ring of it.

Motivating use case:

include(ExternalProject)
ExternalProject_Add(
    outcome URL https://dedi4.nedprod.com/static/files/boost.outcome-v1.0-source-latest.tar.xz
    CONFIGURE_COMMAND "" BUILD_COMMAND "" INSTALL_COMMAND ""
)

add_executable(main main.cpp)
set_property(TARGET main PROPERTY CXX_STANDARD 14)
target_include_directories(main PUBLIC "${CMAKE_CURRENT_BINARY_DIR}/outcome-prefix/src/outcome/include")

The following program crashes:

ned::promise<int> p;
auto t = p.get_future();
t.then([](ned::future<int> const&) {});
p.set_value(0);

The reason is: in future.hpp line 771, it checks v.is_ready(), and in monad.hpp line 866, the return value is output_type(), which is incorrectly "not ready", it should return output_type(value) instead.

Also this overload:

• bool set_error_from_exception(std::exception_ptr const& e)

As the CATCH_ALL macro confers no benefit, replace implementation with this API or replace entire macro.

Add FAQ entry on how the throw keyword is never elided by any compiler

• Licensing to be BSL only
• No cmake, just bjam
• No git submodules at all
• std-flavoured, not boost-flavoured

Still to decide:

• Maybe default to Boost.Exception for throws if that doesn't affect STL semantics

Fails to compile due to selecting copy assignment when udt does not provide one:

  expected<udt> n(make_unexpected(std::error_code(ENOMEM, std::generic_category())));
  n = make_unexpected(std::error_code(EINVAL, std::generic_category()));

Fix for this is blocked by issue #11

Current test/constexprs is so borked it's effectively useless. New script above Tom has fixed for me.

https://github.com/BoostGSoC17/static-map/tree/development/test

• Try moving static asserts into a set of CRTP classes at the very front of an inheritance chain to get MSVC to do the right thing.
• Disable outcome<T&> while you're at it.

Notes from review item:

So, I would do something like we have in the documentation for
Boost.Optional.

The initial page is very short. Short description and a really small
example. In fact too small to sell the power of this library:

http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/index.html

And then in a number of subsequent pages, we describe feature by feature
with short examples:

http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/quick_start.html#boost_optional.quick_start.optional_return_values
http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/quick_start/optional_automatic_variables.html
http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/quick_start/optional_data_members.html
http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/quick_start/bypassing_unnecessary_default_construction.html
http://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/quick_start/storage_in_containers.html

I would do the same for Outcome:

In the landing page just give this example:

https://github.com/akrzemi1/__sandbox__/blob/master/outcome_intro2.md

It only introduces outcome::expected (lot of users will not have heard of
std::expected) and the TRY operation to attract attention of more
informed users.

Then in the second page I would provide a number of short capters each with
one annotated example, each introducing one feature:

• one for using payload wit error_code
• one for all TRY operations
• one for interaction with exception_ptr
• one for catching exceptions into outcome
• one for different ways of accessing the state of outcome

If you like this idea, I would volunteer to write up the initial pages.

Regards,
&rzej;

• Tony suggest access_*() for the narrow observers e.g. .access_value()

Also, the static assert on nothrow move is not predicated on if there is a move constructor, not with fallback onto a copy constructor.

The static assert on default constructibility of T is probably redundant.

Also spell out:

• constructors
• assignments
• destructors
• value_type vs raw_value_type etc

Currently it returns no_value_type, not void.
Consider the generic use case, for example:

template<class T>
inline T await_resume(future<T>& fut)
{
    return fut.get();
}

Such code will fail to compile with future<void>.

• is_ready()
• get_error_or()
• get_exception_or()

Both:

• Have narrow "reinterpret_cast" observers
• Misusage is statically checked via __builtin_unreachable()

For static_checked_optional_outcome/static_checked_optional_result::

• Default constructs to empty
• These implicitly convert from static_checked_outcome/static_checked_result but not the other way round

There's no make_ready_future for future<void>.

Besides, currently make_ready_future(t) is implemented as future<T>(t), however, I'd suggest dropping such ctor for future<T>, consider the case for future<void>, there's no corresponding ctor that makes it a ready future, future<void>()? no, the default constructed one is invalid. Such value-init ctor is confusing and inconsistent.

That would be Paul Bristow, in

https://lists.boost.org/Archives/boost/2015/05/222687.php

The following fails to compile:

struct Date              // no default constructor
{                        // (no default date exists)
  explicit Date(int) {}
};

out::expected<Date> fun ()
{
  return Date{1};        // this fails to compile
}

It fires a static_assert:

"value_type must be default constructible (LEWG Expected requirement)"

This is incorrect, as per p0323r1 (see section X.Z.4.1 in the proposed wording) is_default_constructible is only required when someone triggers the default constructor of expected<T>. Otherwise expected would be not usable for any type without default constructor.

Additionally, .error() is supposed to return a reinterpret style reference to the error state, not by value.

Consider the following use case:

ned::promise<int> p;
auto t = p.get_future();
for (int i = 0; i != 65536; ++i)
    t = t.then([](ned::future<int>&& t) { return t.get() + 1; });
p.set_value(0);
std::cout << "ans: " << t.get();

Every new t depends on its previous result, and since the first t is stalled, a long chain of continuation is constructed. The continuation is invoked when the result is set, as the result, the continuation chain is invoked in a tail recursion manner, and the depth in this example is big enough to cause stackoverflow.

The problem (stackoverflow) could be solved though, and I don't consider this a bug in this library, it's a design decision. It's ok that this library doesn't support such a use case and just document the limitation clearly.

What's the intended decision here?

Your documentation sometimes talks about the class extended_error_code which does not exist. Instead, you probably mean error_code_extended.