A few proposals, need guidance from core contributor

[contificate]

I'm wondering if I'd be able to attempt to merge, if possible, a commit where I completely strip out the va_args implementation of Exception. I'm absolutely baffled as to its va_args implementation (a global buffer with a mutex, really?).

My concerns are: breaking macros (some weird nuances regarding template dependent params in some of the existent macros; I believe I've adjusted them now) and/or not quite understanding the intention behind the two exception accessors (what() and GetShortMessage() - in my code they are the same).

I also amended the code in general, e.g. replace empty dynamic throw() specifiers with the preferred noexcept specifier.

My real concern is not only breaking the current build but having to go and manually reformat the throws where they're formatted using format specifiers, my proposal simply uses variadic templates and pack expands into a std::stringstream then dumps it to the internal std::string message. Thus, allowing things like: throw Exception("Some number: ", 4) - will be formatted with whatever format flags (fmtflags) are being used by std::istream (default).

This is my proposal

class Exception : public std::exception {
public:
    template<typename... T>
    Exception(T&&... args) {
        std::stringstream buffer;
        (buffer << ... << args);
        msg = buffer.str();
    }

    // as noted, I've implemented these as essentially the same thing
    const char* what() const noexcept; // c_str();
    const std::string& GetShortMessage() const noexcept;

private:
    std::string msg;
};

The variadic template parameters, being universal references, would also allow some form of (admittedly odd) dependency inversion for any form of deeper reflection via std::forward (which will move what's passed to the exception - despite the fact I've not seen any explicit attempts to implement move semantics in the codebase). The real concern with such an implementation using pack expansions with stream operators is that it would technically require -std=c++1z. So, we'd need to hear objections before allowing such a build standard.

I'd also like to replace IStream's methods with facade (purely virtual) methods that we will use a template method to access (since, of course, such methods cannot be virtual).

Roughly something like this:

class IStream {
public:
    // purely virtual facade
    virtual void Read(char* dst, const size_t len) = 0;

    template<typename T>
    T Read() {
        T buffer;
        // call purely virtual method implemented by anyone using IStream interface
        Read(reinterpret_cast<char*>(&buffer), sizeof(T));
        return buffer; // use static_asserts to avoid any undesirable reference type copy semantics (despite copy elision - impose - going forward in C++20, we could impose stronger type axioms in the form of C++20's concepts axioms)
    }

};

This provides a more generic implementation that, I believe, would serve the intentions of the project more accurately.

I've omitted the range of static_asserts we could add for returning fundamental types (to avoid copying reference types). This would provide a better implementation since upcasting from a "byte" (unsigned char) to a signed int is an undesirable implementation for an interface.

More generic things I'd like to do:

• Impose better const correctness, many accessor methods are non-const.
• Replace all typedefs with alias declarations
• Stop passing std::string by value, pass by const reference as that would allow you to pass both lvalues and rvalues (since const referencess can bind to rvalues)

I'd like to hear what people think of these proposals (rather than just implementing them, potentially breaking things) then making a vague pull request.

Thanks.

[yvt]

Hi,

I wrote most of the code in Exception.(cpp|h), which has been (unfortunately?) remained mostly intact since the first public release of this project. And I have to admit that this is poorly designed. There is no actual design intent about how exceptions are used; when something goes wrong, you just throw an exception. At least it seems I had one thing in mind - making sure bug reports can be generated with abundant information, no matter what kind of error has occurred, no matter whether debug symbols are available.

I gladly approve your proposal about the exception. But before merging that, I think there are some issues that need to be addressed:

• Fold expressions are C++1z feature, and according to this page they are not supported by any known version of MSVC yet. Please rewrite it so it compiles on MSVC 2015. Otherwise, someone would have to revise the external library pack and the build instructions.
  • I'm not sure about the status of GCC. How many people still use GCC 4.x?
• So, you decided to ditch that infamous back-trace collecting mechanism? I remember it actually helped diagnosing some issues (I don't exactly remember which one, though), but on the other hand I also feel that it is already losing its original significance as the code base gets mature and fewer bugs are reported.
  • In this case, escalating serious/unrecoverable errors to abort or something (instead of C++ exceptions) would be a good idea. We already have an unhandled exception handler that generates a mini-dump file, which can later be used to recover the full back-trace when combined with debug symbols.

I'd also like to replace IStream's methods with facade (purely virtual) methods that we will use a template method to access (since, of course, such methods cannot be virtual).

Looks good to me.

• Impose better const correctness, many accessor methods are non-const.
• Replace all typedefs with alias declarations
• Stop passing std::string by value, pass by const reference as that would allow you to pass both lvalues and rvalues (since const referencess can bind to rvalues)

There is no reason not to approve these proposals. Much appreciated!

You can open a PR before it's ready to merge so we can review as you make changes. Just put "WIP: " in the PR's title.