ned14 / status-code

Proposed SG14 status_code for the C++ standard
Other
64 stars 13 forks source link

Example is bad #27

Closed qknight closed 3 years ago

qknight commented 3 years ago

I try to understand how to add proper error handling by looking at: https://github.com/ned14/status-code/blob/b70d070c02fe74f85d1a05f91b1d8c1dbf7168b0/example/file_io_error.cpp#L172-L190

But that example only handles the error case, can you extend the code so it also does something useful?

Expected behavior: the open_resource should return a file handle so I can read/write something. Instead it has a LINE and FILE, what is that used for?

ned14 commented 3 years ago

You're right that that example isn't much use. It was intended for a WG21 audience only.

I'll delete it shortly, is the example in the Readme.md okay for you?

qknight commented 3 years ago

For some time I was under the impression that the error would contain the actual return value of the function but could not see how this would be implemented. I guess this was never the plan, right? The concept is always what is shown in the Readme.md:

system_error2::system_code sc;  // default constructs to empty
native_handle_type h = open_file(path, sc);
// Is the code a failure?
if(sc.failure())
{
  // Do semantic comparison to test if this was a file not found failure
  // This will match any system-specific error codes meaning a file not found
  if(sc != system_error2::errc::no_such_file_or_directory)
  {
    std::cerr << "FATAL: " << sc.message().c_str() << std::endl;
    std::terminate();
  }
}

Where the sc is passed in as an additional value and processed before using the return value.

ned14 commented 3 years ago

The WG21 example was for exposition, showing how lightweight exception throws of std::error would work.

You're right that for current C++, either you pass in a lvalue ref as a parameter, or use Experimental.Outcome to return an outcome::experimental::result<T> whose alternate E value is an error. So the Result would be T if successful, E if an error.

qknight commented 3 years ago

Thanks for your insights. I guess that using outcome::experimental::result<T> is not straight forward so we bail and use the former method you mentioned.

ned14 commented 3 years ago

Thanks for your insights. I guess that using outcome::experimental::result is not straight forward so we bail and use the former method you mentioned.

Would you be able to clarify how result<T> is not straightforward? It would be useful to learn what the obstacles would be.

I'll reopen this if it's okay, to remind me to go delete that bad example file.

qknight commented 3 years ago

I tried to put together a minimal example and failed since I don't understand your outcome implementation, especially the experimental part of it. Also I failed to get the template magic together.

Here is some pseudo-code I want to illustrate the concept I was trying:

// probably include the generated header from here i suppose...
// https://github.com/ned14/outcome
#include "outcome.hpp"

outcome::experimental::result<T> moveMountain(bool input) {
  if (input == true) {
    return "worked";
  } else {
   // system_error2::system_code sc;  // default constructs to empty
   return system_error2::errc::no_such_file_or_directory
  };
};

outcome::experimental::result<T> ret = moveMountain(true);
... check returned type and print the result ...
... probably with logic from here: https://stackoverflow.com/questions/13636540/how-to-check-for-the-type-of-a-template-parameter ...
if (ret is of type E)
  std::cerr << "error happened" << ret..message().c_str() << std::endl;
else
  std::cout << ret;

outcome::experimental::result<T> ret = moveMountain(false);
if (ret is of type E)
  std::cerr << "error happened" << ret..message().c_str() << std::endl;
else
  std::cout << ret;

Can we create a minimal example from that? Thanks for your help, btw!

qknight commented 3 years ago

It would also help if the two versions of the documentation, both links below, would actually server the same content:

And if the github pages webpage (second link) could container a link to the source code (first link).

ned14 commented 3 years ago

How about this https://godbolt.org/z/Tbr5h7:

#include "outcome-experimental.hpp"

#include <iostream>
#include <string>

namespace outcome_e = OUTCOME_V2_NAMESPACE::experimental;

template<class T> using result = outcome_e::status_result<T>;

result<std::string> moveMountain(bool input) {
  if (input == true) {
    return "worked";
  } else {
   // system_error2::system_code sc;  // default constructs to empty
   return outcome_e::errc::no_such_file_or_directory;
  };
};

void test1()
{
    if(auto r = moveMountain(true))
    {
        std::cout << r.value() << std::endl;;
    }
    else
    {
        // BUG: message() is supposed to print without c_str()!
        std::cerr << "error happened " << r.error().message().c_str() << std::endl;
    }
}

NOTE TO SELF: message() isn't serialising to ostream, and I could have sworn I fixed that six months ago!

Re: those two pages getting out of sync, apologies, the website is manually regenerated using tools on my laptop which I keep forgetting to do. I'll leave this issue open to remind me.

qknight commented 3 years ago

Wow, that works! Thanks!

Can you please add this as a comment to the source code as well:

// Use https://github.com/ned14/outcome#usage-as-a-single-header-file and go to the download page, there is the file outcome-experimental.hpp also.
#include "outcome-experimental.hpp"
ned14 commented 3 years ago

Ok, can you check what I just pushed in that commit there to see if it addresses all your issues?

The only remaining issue is the docs which need to be regenerated. For that I need my personal laptop, so I'll keep this issue open.

ned14 commented 3 years ago

Regened the docs, so closing.