1
\$\begingroup\$

I was thinking about an ExitCode concept in a strict nothrow environment and came up with the following two classes and would like to get some peer review on what flaws they have. They are very simple in terms of inner complexity but are quite complicated in terms of language features (at least for me :) )

ExitCode is a class encapsulating an arbitrary state and a status message.

#include <string>

class ExitCode
{
public:
    enum class TState
    {
        OK,
        FAILED,
        CORRECTED
    };


    /**
    * Default failing exit code, can be created from a simple string
    */
    ExitCode(const std::string& msg = "", const TState state = TState::FAILED)
        : m_sMessage(msg), m_state(state)
    {
    }

    ExitCode(const TState state) : m_state(state)
    {
    }

    /**
    * Uses nested exit code message and adds it to the outer message
    */
    ExitCode(const std::string& msg, const TState state, const ExitCode& nested) :
        m_sMessage(msg + ": " + nested.m_sMessage), m_state(state)
    {
    }

    ExitCode(ExitCode&& other) // Note: VS 2012 has no noexcept
    {
        m_state = other.m_state;
        m_sMessage = std::move(other.m_sMessage);
    }

    ExitCode& operator=(ExitCode&& other) // Note: VS 2012 has no noexcept
    {
        if (this != &other)
        {
            m_state = other.m_state;
            m_sMessage = std::move(other.m_sMessage);
        }

        return *this;
    }

    const std::string& GetMessage() const
    {
        return m_sMessage;
    }

    TState GetState() const
    {
        return m_state;
    }

private:

    std::string m_sMessage;
    TState m_state;
};

Simple cases could be

ExitCode foo1("All failed");

ExitCode foo2("All good", ExitCode::TState::OK);

ExitCode root("All good, but was", ExitCode::TState::OK, foo);

And a derived Status<T> class which is also able to provide some return value. The idea is that some method X may or may not return a valid output but at the same time may or may not return an error (which should not be propagated via exceptions)

template <typename TResult>
class Status : public ExitCode
{
public:

    // Converting
    Status( const TResult& result, const std::string & msg = "", const TState state = TState::OK ) 
        : ExitCode( msg, state ), m_result( result )
    {
    }

    // Converting move
    Status( TResult&& result, const std::string & msg = "", const TState state = TState::OK )
        : ExitCode( msg, state ), m_result( std::move( result ) )
    {
    }

    // Converting
    Status( const std::string& msg = "", const TState state = TState::FAILED ) 
        : ExitCode( msg, state )
    {
    }


    TResult& GetResult() const
    {
        return m_result;
    }

private:

    TResult m_result;
};

Some usage could be

Status<int> s1 = 23;

Status<int> corrected( 1, "Failed to produce valid output, but corrected something", ExitCode::TState::CORRECTED );

Status<int> s2 = "This was bad";

Are there any obvious bad design decisions in such an implementation?

I am currently aware of one: Multiple implicit string conversion when I write code like that:

Status<std::string> status = "Which constructor to use?";
\$\endgroup\$
1
\$\begingroup\$

Overall I would say this is good code. Still, there are a few things that you should (or, at least, could) improve:

  1. TResult& GetResult() const is a bug. This should read TResult const& GetResult() const (and if you had done more testing, your compiler likely should have told you).
  2. You are missing an include (#include <utility>) for std::move.
  3. Depending on how you intend to use these classes, you should add a virtual destructor to ExitCode. The reason is that, if you do fancy polymorphism and try to delete an instance of Status through a pointer to ExitCode, you get undefined behavior. If you don't care about the possible polymorphism, you should be fine as-is.
  4. I share your worry about implicit conversions. I would consider making your constructors explicit.
  5. Keep your formatting consistent. Although your formatting is almost always the same, there is one place where you diverge from it, namely const std::string & msg = "" in the first two constructors of Status. Why do you have a space between std::string and &? (I suppose it's just some oversight of yours).
  6. Why is the default value to two of the constructors of Status for the parameter state TState::OK, but the last constructor has TState::FAILED instead? This is very likely to cause confusion.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Whitespace formatting is indeed a typo. Forgetting a virtual dtor however is bad, and I feel bad :) . Could you elaborate on 1.? I deliberately chose a non const return because I am sure that I won't be able to rely on const-only members of TResult and I wanted to avoid more copies. If Status is a temporary and I only store the result of GetResult I will get undefined behaviour, right? I really think to additionally add a Result const&& GetResult() const to have a "move" getter. \$\endgroup\$ – Samuel Dec 2 '17 at 11:05
  • \$\begingroup\$ @Samuel Sure. I think you're confusing things here a bit. First off, the code as you currently have it is not legal C++, and if you had actually tested that method, your compiler should have complained to you (see this). Your explanation about temporaries is exactly backwards: non-const references do not extend the lifetime of the underlying object, but const references do! Thus, your current code is likely to cause UB sooner or later (if it compiled, that is). About copies, neither & nor const& will incur any copies. Also, const&& is almost never useful. \$\endgroup\$ – Ben Steffan Dec 2 '17 at 11:44
  • \$\begingroup\$ I will look into your suggestions a bit later but I've not only compiled my code as is, I also ran several unit tests on it (successful). I did it with VS 2017 retargeting VS 2012 so it looks like it works for MSVC that way but I will definitely take your suggestions into consideration. \$\endgroup\$ – Samuel Dec 2 '17 at 12:02
  • \$\begingroup\$ @Samuel Have you tested this exact method? I'm a little astonished, as both gcc and clang reject your code (as you can see from the link I gave you in the last comment). \$\endgroup\$ – Ben Steffan Dec 2 '17 at 12:45
  • \$\begingroup\$ I have only VS 2017 here and indeed it does not compile. I will check Monday at work for 2012 because I am quite sure that I am already using Status<T> in some places where I need GetResult but I might simply remember it wrong. \$\endgroup\$ – Samuel Dec 2 '17 at 12:52
0
\$\begingroup\$

Semantics

My opinion is that it interferes with multiple language features, such as brace delimited return:

template <typename InputIt>
std::vector<int> create_v(InputIt first, InputIt last)
{
    return {first, last};
}

With your class, it won't be possible if vector would have private copy and move constructors. Using your State class is impossible in such a case. I'm afraid you'll need language support for this, not library support. Also, if something it not copyable or moveable at all, then it simply won't compile.

The solution might be to add a constructor that would forwards arguments, but then you'd have to drop default argument.

Anyway, as stated in How to submit a proposal, creating essentially a language feature is not easy task.

Alternative

My approach would be to keep one global status code, like C does. Of course this makes it boil down to C, but that's what code in the post intends to do: exclude possibility of exceptional control flow.

Holes in type system

One can do something like this:

Status<std::istream&> stream_status(std::ifstream{"input.txt"});
stream_status.get_result() >> x;

Which is of course undefined behavior. I've fallen into this trap myself a while ago. One can use some template metaprogramming, but I think it makes obvious that this class does not play well with C++. May be in languages with reference only semantics this would work, but not in C++.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ While my code looks portable I could not test brace delimited returns due to my compiler limitation of VS 2012 with very limited C++11 and upwards support. This is a valid consideration. Could you elaborate on the hole? I fail to see the undefined behaviour here. \$\endgroup\$ – Samuel Dec 2 '17 at 11:10
  • \$\begingroup\$ @Samuel, sorry to keep you waiting. The problem is that std::ifstream object is created, but then immediately destroyed after the first line, because nothing took ownership of it. As a result, using stream_status's result is undefined behavior. \$\endgroup\$ – Incomputable Dec 2 '17 at 19:50
  • \$\begingroup\$ Downvoter, could you please explain what is wrong with my answer? I'm happy to improve it if there are some serious content issues. \$\endgroup\$ – Incomputable Dec 2 '17 at 19:51
  • \$\begingroup\$ I will think about passing T& into it. Thanks for the hinting. But I quote Stroustrup here :) : "C makes it easy to shoot yourself in the foot; C++ makes it harder, but when you do it blows your whole leg off" \$\endgroup\$ – Samuel Dec 3 '17 at 10:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.