-
-
Notifications
You must be signed in to change notification settings - Fork 369
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix Issue 1918 - __traits(getVirtualFunctions) returns final functions
- Loading branch information
1 parent
9d33103
commit d096698
Showing
1 changed file
with
2 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's that supposed to mean? Final functions aren't virtual unless they override something!
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, final functions will always have a vtable slot in D.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch. This is a bug. Has it been submitted yet?
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it is, and the code just dates from when there was no final attribute. But then again, judging by Walter's commit above...?
No bug report I've ever seen. I only ran into it with my c++ linking experiments. The compiler always uses a direct call on final functions, so it's strictly an abi problem. (and __traits(getVirtualFunctions) is correct in returning final functions)
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also a program size issue - if a function is in the vtable it will be transitively linked in even if never used.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely must fix this.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll adapt my patch tomorrow. It's worth pointing out that the new getVIrtualFunctions will do exactly the same thing as the new getVirtualMethods once this change is made.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's perfect for backward compatibility. Thanks!
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final functions that do not override anything do not get a vtbl[] slot.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so should we roll this in as is?
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The changes are incorrect. Final functions are virtual if they override something. Not if they do not.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wait, I recall you argued to keep this as is for backwards compatibility. (I'm referring strictly to the doc changes.)
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so my understanding is that I merge this doc change.
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write it as: "Note that non-overriding final functions are considered virtual by this trait. Use getVirtualMethods and isVirtualMethod instead."
d096698There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, looking at the object files it seems final functions that don't override anything indeed don't get a vtbl slot. The error I found was just a problem c++ non-virtual member functions, which are not currently supported anyway.