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

Explicitly defaulted default/copy/move constructors are treated as present #815

Closed
bsilver8192 opened this issue Feb 17, 2022 · 9 comments
Closed
Labels
bug Something isn't working

Comments

@bsilver8192
Copy link
Contributor

Expected Behavior

They should be handled the same as implicitly defaulted constructors (aka no user-defined ones). This means they may be deleted or present, depending on the members and bases.

Actual Behavior

autocxx always tries to call them, even if they're actually deleted.

I'm fixing lots of other kinds of trickiness with implicitly defaulted constructors right now, but I think this one requires extra bindgen annotations so I'm going to leave it for now.

@adetaylor
Copy link
Collaborator

Ah, it was kind of amazing that nobody spotted bugs in that constructor-related logic until now. Good spot.

@bsilver8192
Copy link
Contributor Author

Note that Clang has a default-enabled -Wdefaulted-function-deleted which warns about C++ code triggering this. I think that replacing = default with = delete has the exact same behavior in C++ while avoiding this limitation.

@adetaylor adetaylor added the bug Something isn't working label Feb 24, 2022
bsilver8192 added a commit to bsilver8192/autocxx that referenced this issue Feb 27, 2022
This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a `generate!`/`generate_pod!`.

Added TODOs for the following unhandled parts:
* google#815 (this is a Clang warning anyways, TODOs show all
  the places to change to fix it)
* google#816 (this just means we ignore some implicit
  constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:
* google#832 (this one affects lots of areas)
* google#829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:
* Use the correct C++ name for destructors of nested types
* Handle trait methods for types that end up ignored
bsilver8192 added a commit to bsilver8192/autocxx that referenced this issue Feb 28, 2022
This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a `generate!`/`generate_pod!`.

Added TODOs for the following unhandled parts:
* google#815 (this is a Clang warning anyways, TODOs show all
  the places to change to fix it)
* google#816 (this just means we ignore some implicit
  constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:
* google#832 (this one affects lots of areas)
* google#829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:
* Use the correct C++ name for destructors of nested types
* Handle trait methods for types that end up ignored
@youben11
Copy link

youben11 commented Oct 21, 2022

I tried to follow referenced PRs/issues to know whether the bug is actually fixed or not, but I couldn't be certain. So I'm using the latest version (0.22.4) and I observe a similar behavior, as autocxx generates code that tried to call some copy constructors that were implicitly deleted.

Some context:

/path/lib/Bindings/Rust/target/release/build/project-a995ebfc5d7ae541/out/autocxx-build-dir/include/autocxxgen_ffi.h:110:231: error: use of deleted function ‘mlir::DiagnosticEngine::DiagnosticEngine(const mlir::DiagnosticEngine&)’
  110 | e_new_synthetic_const_copy_ctor_0x69422d23b5144fe1_autocxx_wrapper(mlir::DiagnosticEngine* autocxx_gen_this, const mlir::DiagnosticEngine& arg1)  { new (autocxx_gen_this) mlir::DiagnosticEngine(arg1); }
      |                                                                                                                                                                                                       ^

In file included from /path/llvm-project/mlir/include/mlir/IR/Operation.h:18,
                 from /path/llvm-project/mlir/include/mlir/IR/OpDefinition.h:23,
                 from /path/llvm-project/mlir/include/mlir/IR/Builders.h:12,
                 from /path/lib/Bindings/Rust/target/release/build/project-a995ebfc5d7ae541/out/autocxx-build-dir/cxx/gen0.cxx:2:
/pathl/llvm-project/mlir/include/mlir/IR/Diagnostics.h:403:7: note: ‘mlir::DiagnosticEngine::DiagnosticEngine(const mlir::DiagnosticEngine&)’ is implicitly deleted because the default definition would be ill-formed:
  403 | class DiagnosticEngine {

@adetaylor
Copy link
Collaborator

@youben11 I don't think this is believed to be fixed yet.

@youben11
Copy link

thanks @adetaylor. So the current workaround would be to explicitly mark them deleted in Cpp, but is there a workaround in rust (autocxx) if I can't touch the Cpp code?

@adetaylor
Copy link
Collaborator

I think all you can do is explicitly block!(mlir::DiagnosticEngine::DiagnosticEngine) but I appreciate this may make autocxx useless for your needs.

@youben11
Copy link

I didn't knew there was a way to do it! This is actually awesome! I don't need this specific type (mlir::DiagnosticEngine), so I can definitely block it as far as it doesn't affect my other generated types, which is the case as far as I understand.

@adetaylor
Copy link
Collaborator

Some work in progress:

These two PRs don't yet connect together: although autocxx now knows about defaulted functions, it doesn't treat them any differently than it does now. Also, the test case is very much incomplete.

@adetaylor
Copy link
Collaborator

I think this was fixed in #1180.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants