autocxx
autocxx copied to clipboard
autocxx does not recognize concrete subclasses of abstract base classes
Minimal Working Example
(I think it's minimal. I'll try to add a pull request with a new test in due course.)
Test.h
#pragma once
namespace test {
class A;
class B;
}
namespace test {
class A {
public:
A() {}
~A() {}
// the following line makes Test2 Opaque, and deprives it of a make_unique()
// comment next line, uncomment the one after to obtain a make_unique() for Test2
virtual int b() = 0;
// int b() { return 2; }
};
class B: public A {
public:
B() {}
~B() {}
int b() { return 3; }
};
}
build.rs
fn main() {
let path = std::path::PathBuf::from("src");
let mut b = autocxx_build::Builder::new("src/main.rs", &[&path]).expect_build();
b.flag_if_supported("-std=c++14").compile("autocxx-demo");
println!("cargo:rerun-if-changed=src/main.rs");
}
main.rs
use autocxx::prelude::*;
include_cpp! {
#include "Test.h"
safety!(unsafe)
generate!("test::B")
}
fn main() {
// I'd like to initialize a B here, but I can't
}
Expected Behavior
The generated struct B
has a make_unique
implementation.
Actual Behavior
The generated struct B
has a Drop
implementation but no make_unique
. It is not clear how I could instantiate a variable of type B
.
Steps to Reproduce the Problem
See above
Specifications
- Version: 0.16.0 (or later if there's a later)
- Platform: Windows
Thanks. Yep, a pull request with a test case would be greatly appreciated and will likely yield a quicker fix. Nevertheless thanks for the minimal test case.
I've added an integration test; see pull request #775 .
Thanks for the test!
OK, an initial diagnosis by running your test and looking at the diagnostics. I'd argue that we have three separate bugs here.
-
autocxx
currently requires all base classes to be specified on the allowlist asgenerate!
items. Bug 1 is that we failed to document that requirement. - Bug 2 is that this requirement was based on a flawed assumption, which is that we wouldn't be told about base classes by
bindgen
unless they were allowlisted. It does rather look likebindgen
tells us about base classes automatically, so it's quite possible that this requirement can simply be removed. - Bug 3, however, is that it still doesn't work even if both
A
andB
are given on the allowlist. This is because we don't currently detect subclasses which have concrete implementations of pure virtual base class functions. This is just an omission from our feature set.
I plan to confirm my understanding regarding Bug 2, and if so, remove this code that relates to allowlisting base classes. Obviously if all's well, that renders the documentation change ("bug 1") unnecessary.
If all's well there, I'll go ahead and tackle Bug 3 which shouldn't be drastically hard.
It would be great if you can accept the CLA so I can land your test. Thanks again for the report!
I had to create a new pull request in order to accept the CLA. Sorry for the inconvenience, but the new one allowed me to accept the CLA and is otherwise identical.
No problem at all, sorry for the inconvenience of the CLA.
I have fixed "Bug 2" (which renders "Bug 1" irrelevant). This is already a usability improvement for autocxx users, so thanks for that.
Bug 3 will be a little more involved as we'll have to track which pure virtual functions have been overridden and therefore the net 'abstract'ness of each type. If anyone reading this wants to have a go at it, the code is in autocxx-engine/src/conversion/analysis/abstract_types.rs
- the loop around line 57 needs to be much more sophisticated.