Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

object.di: Rename TypeInfo_Const.next to TypeInfo_Const.base. #1182

Closed
wants to merge 1 commit into from

Conversation

redstar
Copy link
Contributor

@redstar redstar commented Mar 1, 2015

This is used e.g. in rt.lifetime, function unqualify().

@redstar
Copy link
Contributor Author

redstar commented Mar 1, 2015

Could someone explains what is going on here? The original TypeInfo_Const from object.di has only a next property but no base field. rt.lifetime should therefore not compile.

@rainers
Copy link
Member

rainers commented Mar 1, 2015

I'm very much in favor of this change, see also https://issues.dlang.org/show_bug.cgi?id=3806 and https://issues.dlang.org/show_bug.cgi?id=8656. See https://github.com/D-Programming-Language/druntime/blob/master/src/rt/aaA.d#L696 for the usual dance to support both object.d and object.di.

@rainers
Copy link
Member

rainers commented Mar 1, 2015

Could someone explains what is going on here? The original TypeInfo_Const from object.di has only a next property but no base field. rt.lifetime should therefore not compile.

Next is a virtual member function in TypeInfo, so the next property changes the meaning depending on whether you import object.di or object.d.

@rainers
Copy link
Member

rainers commented Mar 1, 2015

I guess you'll have to change the test to base, too. I'm not sure why nextdoesn't work there, too.
There is also at least https://github.com/D-Programming-Language/phobos/blob/master/std/format.d#L5387 affected, it will change slightly.

@redstar
Copy link
Contributor Author

redstar commented Mar 1, 2015

This explains the test case failure somehow, but why does rt.lifetime compile? Every module import object.di but there is no base element in the current source...

@rainers
Copy link
Member

rainers commented Mar 1, 2015

This explains the test case failure somehow, but why does rt.lifetime compile? Every module import object.di but there is no base element in the current source...

druntime is compiled with object.d on the command line, so it does not import object.di.

@redstar
Copy link
Contributor Author

redstar commented Mar 1, 2015

Cool. If I run

dmd -c src\rt\lifetime.d -I src

then I get the error

src\rt\lifetime.d(202): Error: no property 'base' for type 'object.TypeInfo_Const'
src\rt\lifetime.d(204): Error: no property 'base' for type 'object.TypeInfo_Invariant'
src\rt\lifetime.d(206): Error: no property 'base' for type 'object.TypeInfo_Shared'
src\rt\lifetime.d(208): Error: no property 'base' for type 'object.TypeInfo_Inout'

I try to change the failing test.

@schveiguy
Copy link
Member

So if I understand this correctly, TypeInfo_Const defines base in object_.d, but next in object.di?

I think we need at least an alias to next. TypeInfo_Const is a public API, people may use the next field in their code.

Is there a reason not to just change object_.d to have a next field instead? That would be a less breaking change.

@rainers
Copy link
Member

rainers commented Mar 3, 2015

Is there a reason not to just change object_.d to have a next field instead? That would be a less breaking change.

That's possible, but it makes for a horrible API. The meaning of next changes whether you are using it on a TypeInfo_Const or a TypeInfo:

import std.stdio;

void main()
{
    const int[long] aa;
    pragma(msg,typeof(aa)); // const(int[long])

    auto cti = typeid(aa);
    writeln(cti);           // const(const(int)[long])
    writeln(cti.next);      // const(int)[long]
    TypeInfo ti = typeid(aa);
    writeln(ti);            // const(const(int)[long])
    writeln(ti.next);       // const(int)
}

@Orvid
Copy link
Contributor

Orvid commented Mar 3, 2015

That looks more like a bug than something that is done intentionally...

@rainers
Copy link
Member

rainers commented Mar 3, 2015

That looks more like a bug than something that is done intentionally...

Yes, but someone might still have used it. To avoid silent change, we might want to add

@property @disable override deprecated("use base") inout(TypeInfo) next() nothrow pure inout @nogc;

to TypeInfo_Const for some tme.

@rainers
Copy link
Member

rainers commented Mar 3, 2015

I just notice that TypeInfo_Delegate and TypeInfo_Function have the same issue. They don't use different identifiers, but just redeclare next in the derived class both in object.di and object_.d.

@Orvid
Copy link
Contributor

Orvid commented Mar 7, 2015

As a random question, why do we need object.di to begin with? Is there any reason we can't just use object.d instead?

@MartinNowak
Copy link
Member

As a random question, why do we need object.di to begin with? Is there any reason we can't just use object.d instead?

That's an old separation to hide implementation details. It's still a bit relevant because object is imported by every D code, but we might reconsider if it's still necessary.

@MartinNowak
Copy link
Member

You need to fix this dmd test to make it pass.

@MartinNowak
Copy link
Member

@Property @disable override deprecated("use base") inout(TypeInfo) next() nothrow pure inout @nogc;

Good idea @rainers.

@redstar
Copy link
Contributor Author

redstar commented Apr 11, 2015

This is really broken. The problem is that rt.lifetime relays in the implementation details of object_d.. It uses the next property (which returns the next-after-next typeinfo in case of a qualifier) to drop a possible qualifier and the array type info from the chain. It uses base to unqualify the base type then.

On the other hand https://github.com/D-Programming-Language/dmd/blob/714d1b32cfe721e6266ebbc096355c8ce109f8b8/test/runnable/testtypeid.d#L439 requires the next field from the interface object.di.

With my changes it now passes all Phobos unit tests and the mentioned dmd test.

redstar added a commit to ldc-developers/druntime that referenced this pull request Apr 11, 2015
Better solution is to change rt.lifetime.

See dlang#1182 for more info.

Fixes failing test runnable/testtypeid.d
@MartinNowak
Copy link
Member

I think next comes from the compiler idea of TypeNext, so TypeInfo_Const should use base and override next() as base.next().
The dmd test shouldn't use next at all and needs to be fixed.

@@ -1225,7 +1225,7 @@ class TypeInfo_Const : TypeInfo
override @property size_t tsize() nothrow pure const { return base.tsize; }
override void swap(void *p1, void *p2) const { return base.swap(p1, p2); }

override @property inout(TypeInfo) next() nothrow pure inout { return base.next; }
override @property inout(TypeInfo) next() nothrow pure inout { return base; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a desired change, at least it might break more code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes can you please undo that.

This is used e.g. in rt.lifetime, function unqualify.
@redstar
Copy link
Contributor Author

redstar commented May 1, 2015

I removed the change to next(). @rainers already fixed the broken dmd test. Thanks!

But I still think that the TypeInfo.next() semantic is broken. Or should people understand the compiler source just to do compile time reflection?

@rainers
Copy link
Member

rainers commented May 1, 2015

I removed the change to next(). @rainers already fixed the broken dmd test. Thanks!

I think you can remove the additional calls to unqualify in unqualify(ti).next, too. TypeInfo_Const.next() already does the removal of qualifiers and is slightly more efficient in the most common case, i.e. when there is no qualifier to begin with.

But I still think that the TypeInfo.next() semantic is broken. Or should people understand the compiler source just to do compile time reflection?

There is no strict definition of what next is supposed to do, but I think it is consistently returning the type after going through an indirection.

@rainers
Copy link
Member

rainers commented May 9, 2015

I guess this is obsolete now that we are always using object_.d to begin with.

@rainers rainers closed this May 9, 2015
@redstar redstar deleted the object branch January 10, 2016 02:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants