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

Fix bugzilla issue# 24864: hasElaborateDestructor wrong with anonymous unions #17075

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Nov 18, 2024

hasElaborateDestructor should only be true if the type actually has a destructor which is called when the object leaves scope. Anonymous unions do not give a destructor to their containing type even if one or more of their members has one (the same with copy constructors, postblit constructors, and assignment operators). For that sort of thing to work properly, such functions need to be added manually to the struct such that they call the functions appropriately on whichever member of the union is the valid one.

As such, hasElaborateDestructor should not be true based on the member of a union, and there is no need to check the fields of a struct at all, because the ultimate question is whether the struct itself has a destructor. So, the fact that the code has been checking the struct's members is unnecessary in general and wrong in the case of anonymous unions.

The change to checking for __xdtor is because __dtor is an explicitly declared destructor, whereas __xdtor is generated by the compiler (either because the struct has a destructor or because it has at least one member variable which does - and which isn't in a union). So, the check for whether the member variables had a __dtor member was probably to try to catch the cases where the struct hadn't declared an explicit destructor but had had one generated because of its member variables. However, simply checking for __xdtor catches that along with explicit destructors, and there's no need to instantiate any additional templates to check the member variables.

In addition to fixing this issue with hasElaborateDestructor, I've improved the tests for hasElaborateCopyConstructor and hasElaborateAssign to catch the same issue for them, though they don't currently have the bug.

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 18, 2024

Thanks for your pull request, @jmdavis!

Bugzilla references

Auto-close Bugzilla Severity Description
24864 normal hasElaborateDestructor incorrectly true for structs with anonymous unions

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#17075"

@jmdavis
Copy link
Member Author

jmdavis commented Nov 18, 2024

It would appear that there is a bug with __traits(hasMember, ...) which gets triggered when the layer of types is deep enough: #17075

So, I'm going to have to rework my fix to use __traits(allMembers, ...), which kind of sucks. :|

@jmdavis
Copy link
Member Author

jmdavis commented Nov 18, 2024

Okay. Hopefully, that commit fixes the issue. I checked __traits(allMembers, ...) using an array, since that reduces the number of template instantiations in comparison to using a template to do the search, and as I understand it, it's generally faster to use CTFE than recursive templates. Either way, it would be nice if the compiler bug were fixed so that the simple __traits(hasMember, ...) check would work.

… unions

hasElaborateDestructor should only be true if the type actually has a
destructor which is called when the object leaves scope. Anonymous
unions do not give a destructor to their containing type even if one or
more of their members has one (the same with copy constructors, postblit
constructors, and assignment operators). For that sort of thing to work
properly, such functions need to be added manually to the struct such
that they call the functions appropriately on whichever member of the
union is the valid one.

As such, hasElaborateDestructor should not be true based on the member
of a union, and there is no need to check the fields of a struct at all,
because the ultimate question is whether the struct itself has a
destructor. So, the fact that the code has been checking the struct's
members is unnecessary in general and wrong in the case of anonymous
unions.

The change to checking for __xdtor is because __dtor is an explicitly
declared destructor, whereas __xdtor is generated by the compiler
(either because the struct has a destructor or because it has at least
one member variable which does - and which isn't in a union). So, the
check for whether the member variables had a __dtor member was probably
to try to catch the cases where the struct hadn't declared an explicit
destructor but had had one generated because of its member variables.
However, simply checking for __xdtor catches that along with explicit
destructors, and there's no need to instantiate any additional templates
to check the member variables.

In addition to fixing this issue with hasElaborateDestructor, I've
improved the tests for hasElaborateCopyConstructor and
hasElaborateAssign to catch the same issue for them, though they don't
currently have the bug.
__traits(hasMember, ...) currently, incorrectly, reports that some types
have __xdtor when they don't. So unfortunately, we have to check with
__traits(allMembers, ...) until that's fixed.
@thewilsonator thewilsonator merged commit a4f218a into dlang:master Nov 18, 2024
41 checks passed
@jmdavis jmdavis deleted the hasElaborateDestructor branch November 18, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants