Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors involving system calls should use the <system_error> mechanisms #32

Closed
eyalroz opened this issue May 29, 2022 · 3 comments · Fixed by #45
Closed

Errors involving system calls should use the <system_error> mechanisms #32

eyalroz opened this issue May 29, 2022 · 3 comments · Fixed by #45
Assignees
Labels
enhancement New feature or request

Comments

@eyalroz
Copy link
Contributor

eyalroz commented May 29, 2022

C++11 introduced a mechanism - not a fun one, or an easy one to use, but a mechanism - for working with system errors - among others, the ones that, on Unix, you get via errno, and on Windows, with GetLastError() . This is the <system_error> header.

When the error you encounter is one of those - that's what you should throw. See some SO posts about it:

@martin-olivier
Copy link
Owner

I was thinking to reintroduce an equivalent to dylib::handle_error and dylib::symbol_error to be able to handle correctly each error type, witch is complicated actually since those two exceptions are now std::runtime_error

    /**
     *  This exception is raised when the library failed to load 
     *  or encountered symbol resolution issues
     *
     *  @param message the error message
     */
    class handle_error : public std::runtime_error {
    public:
        explicit handle_error(std::string &&message) : std::runtime_error(std::move(message)) {}
    };

    /**
     *  This exception is raised when the library failed to load a symbol. 
     *  This usually happens when you forgot to put <DYLIB_API> before a library function or variable
     *
     *  @param message the error message
     */
    class symbol_error : public std::runtime_error {
    public:
        explicit symbol_error(std::string &&message) : std::runtime_error(std::move(message)) {}
    };

For example

try {
    dylib lib("foo");
    lib.get_variable<double>("pi_value");
}
catch (const dylib::handle_error &) {
    // handle lib loading failure
}
catch (const dylib::symbol_error &) {
    // handle symbol loading failure
}

@eyalroz
Copy link
Contributor Author

eyalroz commented Jun 11, 2022

Well, inheriting std::runtime_error is better than only inheriting std::exception, but I'm always skeptical of introducing new exception classes.

I would try to take inspiration from what other languages use as errors/exceptions when failing to dynamically load compiled code.

@martin-olivier
Copy link
Owner

I've made the following changes in #45 :

Unit tests

I've added this test to check the load of a dynamic library from the system library path :

TEST(system_lib, basic_test) {
#if defined(_WIN32) || defined(_WIN64)
    dylib lib("kernel32");
    lib.get_function<DWORD()>("GetCurrentThreadId");
#elif defined(__APPLE__)
    dylib lib("ssh2");
    lib.get_function<const char *(int)>("libssh2_version");
#else
    dylib lib("pthread");
    lib.get_function<int()>("pthread_yield");
#endif
}

Exceptions

load_error
This exception is raised when the library failed to load or the library encountered symbol resolution issues

symbol_error
This exception is raised when the library failed to load a symbol

Those exceptions inherit from std::runtime_error

try {
    dylib lib("foo");
    double pi_value = lib.get_variable<double>("pi_value");
    std::cout << pi_value << std::endl;
}
catch (const dylib::load_error &e) {
    // failed loading "foo" lib
}
catch (const dylib::symbol_error &e) {
    // failed loading "pi_value" symbol
}

@martin-olivier martin-olivier self-assigned this Jun 13, 2022
@martin-olivier martin-olivier linked a pull request Jun 13, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants