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

C++: write proper bindings #139

Merged
merged 11 commits into from
Jan 4, 2021
Merged

C++: write proper bindings #139

merged 11 commits into from
Jan 4, 2021

Conversation

tschoonj
Copy link
Owner

No description provided.

@tschoonj tschoonj force-pushed the cplusplus branch 2 times, most recently from 350bb07 to 45db990 Compare December 23, 2020 19:10
@lgtm-com
Copy link

lgtm-com bot commented Dec 27, 2020

This pull request introduces 1 alert when merging 9ac4f20 into 327f2e0 - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor

@tschoonj
Copy link
Owner Author

@mschollmeier what do you think about these C++ bindings?

@tschoonj tschoonj force-pushed the cplusplus branch 2 times, most recently from 09fb756 to af8d4c1 Compare December 31, 2020 14:20
@mschollmeier
Copy link
Contributor

@tschoonj That's very close to what I had started, apologies for not finding the time to work on that. Thank you for doing the work!
It looks like you've decided not to use a Python script to generate the c++ code. How come?

I have a few more error codes in process_error, which you could consider adding:

/* process_error translates error codes from xraylib to c++
* and throws the corresponding std exception.
* c++ reference: https://en.cppreference.com/w/cpp/error/exception
* and https://stackoverflow.com/questions/11938979/what-exception-classes-are-in-the-standard-c-library
*/
void process_error(xrl_error *error, std::string funcName) {

    if (!error)
        return;

    switch (error->code) {

    case XRL_ERROR_MEMORY: /* set in case of a memory allocation problem */
        throw std::bad_alloc();
    case XRL_ERROR_INVALID_ARGUMENT: /* set in case an invalid argument gets passed to a routine */
        throw std::invalid_argument(funcName + error->message);
    case XRL_ERROR_IO: /* set in case an error involving input/output occurred */
        throw std::ios_base::failure(error->message);
    case XRL_ERROR_TYPE: /* set in case an error involving type conversion occurred */
        throw std::bad_cast();
    case XRL_ERROR_UNSUPPORTED: /* set in case an unsupported feature has been requested */
        throw std::invalid_argument(funcName + error->message);
    case XRL_ERROR_RUNTIME:
        throw std::runtime_error(error->message); /* set in case an unexpected runtime error occurred */
    }
}

@tschoonj
Copy link
Owner Author

tschoonj commented Jan 3, 2021

Glad to read you like it!

What about the classes I introduced? Do you think that this is sensible?

I decided to skip the Python script and use macros instead, mostly because I thought it was simpler to have just one header file compared to having three files (one handwritten header, one generated header, one python script)...

Unless you have any objections I will merge this PR in. Feel free to then open a PR to modify the error code handling function.

@mschollmeier
Copy link
Contributor

Hi Tom,
I've started to take a closer look at the bindings. I've noticed that you can replace the various _XRL_FUNCTION_nxx macros with just one macro, using a variadic template function:

#define _XRL_FUNCTION(_name) \
    template<typename... T> \
    double _name(const T... args) { \
    xrl_error *error = nullptr; \
    double rv = ::_name(args..., &error); \
    _process_error(error); \
    return rv; \
    }

This compiles without complaints. Spot checks for a few functions yield the correct results, I'll do more testing and if successful submit a pull request.

@tschoonj tschoonj merged commit 473d644 into master Jan 4, 2021
@tschoonj
Copy link
Owner Author

tschoonj commented Jan 4, 2021

Hi Marius,

I have merged this PR in: please base your changes on the updated master branch. That way the C++ tests can be reused and will be run as part of the CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants