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

[DRAFT] Support for C++ Exceptions #1426

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

torrancew
Copy link

Fixes #1412

This is an early draft to solicit feedback. It is functional, but there are a couple of implementation details that felt rather clumsy:

  • Mapping a user specified qualified name (eg Xapian::Database::add_document) to the mangled representations available during function analysis
  • Representing the transformations themselves within the existing machinery (the throwable bit is more or less just bolted on at this phase)

And of course some obvious missing bits like documentation, additional test cases, etc. Let me know what you think about the current approach, and anything you'd like to see improved -- I'm happy to iterate!

This patch introduces a throws! macro, so that individual functions and methods can be marked as throwing an exception. Example usage can be found in the provided unit tests, but generally resembles:

import_cpp! {
    #include "foo.h"
    generate!("do_something")
    throws!("do_something")
}
  • CLA signed
  • Tests pass
  • Appropriate changes to README are included in PR

@torrancew torrancew changed the title [DRAFT] Feature/throws exception [DRAFT] Support for C++ Exceptions Jan 20, 2025
Copy link
Collaborator

@adetaylor adetaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very much along the right lines. In fact you've gone further than I thought you'd do in the initial iteration by tackling the MoveIt TryNew stuff.

Before I land this, I'd want to see:

  • Diagnostics if a throws! directive had no effect
  • Documentation, obviously
  • All existing tests passing (I'll click the Approve and Run button shortly)

Generally speaking this is looking great.

@torrancew
Copy link
Author

Thanks!

The test suite is green locally, and the failing tests appear to be the ones I've added. I'm inclined to suspect a difference in my build environment and CI's. I'll dig into that and try to make them a bit more portable, assuming it's correct. Likewise I'll start in on cleaning up the docs. In the mean time, I added a few more test cases, and in the process uncovered (and fixed) support for constructors as well!

@@ -1196,6 +1198,16 @@ impl<'a> FnAnalyzer<'a> {
}
let mut cxxbridge_name = make_ident(&cxxbridge_name);

let friendly_name = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that much of the naming stuff is a bit of a mess, but please do your best to be consistent here with the way other configuration directives are approached.

In byvalue_checker.rs we have:

        let pod_requests = config
            .get_pod_requests()
            .iter()
            .map(|ty| QualifiedName::new_from_cpp_name(ty))
            .collect();

and the comment in new_from_cpp_name suggests this is what you should be using.

@@ -196,6 +198,16 @@ pub(super) fn gen_function(
// which the user has declared.
let params = unqualify_params(params);
let ret_type = unqualify_ret_type(ret_type.into_owned());

let ret_type = if analysis.throwable {
Copy link
Collaborator

@adetaylor adetaylor Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight preference for a single match statement with an arm something like _ if !analysis.throwable => ret_type. (Here and elsewhere). Just to reduce depth of nested statements.

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.

Feature Request: Exception Support
2 participants