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

object.classinfo: Return a TypeInfo_Class instead of a TypeInfo_Interface #1532

Merged
merged 1 commit into from
Jun 18, 2016

Conversation

kinke
Copy link
Member

@kinke kinke commented May 30, 2016

This may be enough to fix #1471 (at least there's no crash anymore for @rainers' minimal testcase).

@JohanEngelen
Copy link
Member

Could you also add a testcase for this?

Perhaps something like:

// RUN: %ldc -c -output-ll -of=%t.ll %s && FileCheck %s < %t.ll

class Node
{
    final void _clone()
    {
// CHECK-NOT: load %object.TypeInfo_Interface*
// CHECK: load %object.TypeInfo_Class*
        this.classinfo.create();
    }
}

@kinke
Copy link
Member Author

kinke commented May 30, 2016

Thanks for the ready-to-paste test, but I'm not convinced this deserves a test on its own; it rather seems like a bug due to lack of focus or a typo to me, with an extremely low probability of being reintroduced. ;)
Let's see if it passes the tests and whether it also fixes Rainer's problems first.

Edit: But if there's really not a single test accessing a ClassInfo this way, then we definitely need one (preferably upstream).

@JohanEngelen
Copy link
Member

OK. Pretty much the first thing I think of now is: "does it have tests?" More tests, more better ;-)
I am not sure the IR has to be checked, but a simple "does it compile" check would be nice, no? It would check for many other things besides your typo ;)

@kinke
Copy link
Member Author

kinke commented May 30, 2016

I'm pretty sure this typo isn't mine. ;)
Yeah that's what I meant (not checking the IR, just checking whether it compiles and works), but I'd rather see such tests go upstream so that all compilers benefit (I don't know whether GDC uses dmd-testsuite too, but I guess and hope so) from improved test coverage. DMD's test coverage is pretty bad, especially wrt. basic compiler and language stuff - #1327 wouldn't exist if there was a good test coverage for the evaluation order of expressions, which is sadly a constant source of errors.

@JohanEngelen
Copy link
Member

Opps, sorry misread :(
Upstreaming is good.

@rainers
Copy link
Contributor

rainers commented May 30, 2016

LGTM, but I haven't tried it yet (I replaced all occurences with typeid). I wonder why this isn't lowered to TypeidExp in the front end, though.
A test should verify runtime behaviour, too.

@kinke
Copy link
Member Author

kinke commented May 30, 2016

This may not be correct. TypeidExp (thanks for the hint, Rainer) returns a TypeInfo_Interface for interfaces, not a TypeInfo_Class. Don't know if that is supposed to happen for .classinfo too.

@rainers
Copy link
Contributor

rainers commented May 30, 2016

According to the doc and Kenji, typeid() and .classinfo are supposed to do the same. There have been some discussions around what should be returned and that strange things happen with dmd, too: dlang/dmd#4711, https://issues.dlang.org/show_bug.cgi?id=14612 and a number of linked reports.

@kinke
Copy link
Member Author

kinke commented Jun 16, 2016

Updated so that .classinfo for interfaces returns a TypeInfo_Interface, as before and compatible to TypeidExp codegen.

@rainers
Copy link
Contributor

rainers commented Jun 18, 2016

LGTM. It's compatible with dmd with respect to not returning the actual class for interfaces.

@rainers rainers merged commit f06f205 into ldc-developers:master Jun 18, 2016
@kinke kinke deleted the classinfo branch August 24, 2017 19:46
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.

ICE when accessing classinfo (breaks DustMite)
3 participants