https://issues.dlang.org/show_bug.cgi?id=13541

Marco Leise <Marco.Leise@gmx.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |Marco.Leise@gmx.de

--- Comment #1 from Marco Leise <Marco.Leise@gmx.de> ---
That recursion is really bad. :p

> 1. sysErrorString()'s argument should ONLY be values returned by Windows APIs like GetLastError(), and so this should always succeed.

It should, yes. And that's surely Microsofts stance as well. Yet they state that 0 is returned on error, so if this was my code I would at least handle that case and print some generic English message like "Failed to retrieve WinAPI error message for code 0x123456" instead of an empty string.

--

https://issues.dlang.org/show_bug.cgi?id=13541

Vladimir Panteleev <thecybershadow@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thecybershadow@gmail.com

--- Comment #2 from Vladimir Panteleev <thecybershadow@gmail.com> ---
sysErrorString should be replaced with the newly-added wenforce wherever applicable. Some code in Phobos (std.mmfile at least) also incorrectly uses/used errnoEnforce when the error code was stored in GetLastError, not errno.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #3 from Marco Leise <Marco.Leise@gmx.de> ---
When I was writing platform specific code, I just put POSIX and WinAPI enforcement into a single `osApiEnforce`, throwing an OSApiException. I found I used it very often, because it was so easy to remember.

Still we would want to catch that Exception inside sysErrorString() to be able
to make it nothrow.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #4 from Vladimir Panteleev <thecybershadow@gmail.com> ---
(In reply to Walter Bright from comment #0)
> This should be an assert because:
> 
> 1. sysErrorString()'s argument should ONLY be values returned by Windows
> APIs like GetLastError(), and so this should always succeed.

No, it may fail. Windows exposes a SetLastError API, which allows setting error codes which may not be valid. Third-party libraries may use SetLastError to set the error code to a private one, which sysErrorString may fail to parse.

> 2. recursively calling sysErrorString() with the SAME value will cause a stack overflow crash, not any usable exception.

No, sysErrorString is not recursing "with the SAME value". It is recursing with the value from FormatMessageW, which we can expect to be valid.

--

https://issues.dlang.org/show_bug.cgi?id=13541

Jakob Ovrum <jakobovrum@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakobovrum@gmail.com

--- Comment #5 from Jakob Ovrum <jakobovrum@gmail.com> ---
(In reply to Walter Bright from comment #0)
> 1. sysErrorString()'s argument should ONLY be values returned by Windows
> APIs like GetLastError(), and so this should always succeed.
> 2. recursively calling sysErrorString() with the SAME value will cause a
> stack overflow crash, not any usable exception.

As pointed out, it can fail, and it does not recurse with the same value. I should have left a comment when I initially wrote it.

However, I agree with the desire for `nothrow`. I think the practical compromise would be to replace the runtime error check for `FormatMessageW` with an assert (that also uses assert(false) in the release path), then document that the function only accepts valid Windows error codes.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #6 from Vladimir Panteleev <thecybershadow@gmail.com> ---
(In reply to Marco Leise from comment #3)
> When I was writing platform specific code, I just put POSIX and WinAPI enforcement into a single `osApiEnforce`, throwing an OSApiException. I found I used it very often, because it was so easy to remember.

I think it would be nice to make WindowsException and ErrnoException a descendant of a new OSException class. FileException should be an alias of OSException - there really isn't anything specific to file operations (when compared with other syscalls) to warrant an exception class of their own.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #7 from Marco Leise <Marco.Leise@gmx.de> ---
(In reply to Jakob Ovrum from comment #5)
> As pointed out, it can fail, and it does not recurse with the same value. I should have left a comment when I initially wrote it.
> 
> However, I agree with the desire for `nothrow`. I think the practical compromise would be to replace the runtime error check for `FormatMessageW` with an assert (that also uses assert(false) in the release path), then document that the function only accepts valid Windows error codes.

The cleanest would still be to always check the error code from FormatMessage, even in case of correct input, just because it gives no guarantees about success in the documentation.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #8 from Walter Bright <bugzilla@digitalmars.com> ---
Point taken about sysErrorString() and GetLastError() not being infinite
recursion.

But still, FileException should be fixed so it does not throw in the constructor, i.e. rewrite:

    if(length == 0)
    {
        throw new Exception(
            "failed getting error string for WinAPI error code: " ~
            sysErrorString(GetLastError()));
    }

as:

    if(length == 0)
    {
        auto newErrCode = GetLastError();
        assert(errCode != newErrCode);    // no infinite recursion
        return
            "failed getting error string for WinAPI error code: " ~
            sysErrorString(newErrCode);
    }

or something like that.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #9 from Vladimir Panteleev <thecybershadow@gmail.com> ---
Only that won't make sysErrorString nothrow because of UTF conversions.

(In reply to Walter Bright from comment #8)
>         return
>             "failed getting error string for WinAPI error code: " ~
>             sysErrorString(newErrCode);

For the record, this is a horrible solution. Imagine that an end-user tries using a D app, and they get an error popup that just says "failed getting error string for WinAPI error code: Resource not found". Not even an error code they can Google for.

A better solution, and what wenforce already does, is to silently omit the error message and just return the error code as a string.

--

https://issues.dlang.org/show_bug.cgi?id=13541

--- Comment #10 from Sobirari Muhomori <dfj1esp02@sneakemail.com> ---
(In reply to Vladimir Panteleev from comment #9)
> A better solution, and what wenforce already does, is to silently omit the error message and just return the error code as a string.

"Resource not found" can be added as a chained OSException.

--