Fix Issue 18020 - [Reg 2.078] no property opCmp for anon class #7378

JinShil · 2017-11-30T03:41:58Z

ClassKind.anonymous is not a linkage type.

In this case, this should be targeting "master", right?

dlang-bot · 2017-11-30T03:42:00Z

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close	Bugzilla	Description
✓	18020	[Reg 2.078] no property opCmp for anon class

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

WalterBright

Mind the Jenkins failures

jacob-carlborg · 2017-11-30T08:21:37Z

ClassKind.anonymous is not a linkage type.

Technically classes don't have linkage a type, only functions do. In the previous PR #7315, a couple of mutually exclusive booleans were converted to a single enum to actually enforce the exclusiveness. For example, a class cannot be anonymous and C++ or Objective-C at the same time.

JinShil · 2017-11-30T08:34:39Z

a class cannot be anonymous and C++ or Objective-C at the same time.

... but it can be ClassKind.d and anonymous at the same time, right? Hence the regression. I think the previous PR had too much refactoring in it and navigated away from the issue at hand.

RazvanN7

This seems to be the right way, but please also fix the issue stated in #7315

jacob-carlborg · 2017-11-30T09:21:28Z

but it can be ClassKind.d and anonymous at the same time, right?

Yes.

JinShil · 2017-11-30T09:33:23Z

but please also fix the issue stated in #7315

Done.

jacob-carlborg · 2017-11-30T09:39:46Z

src/ddmd/todt.d

@@ -719,7 +719,7 @@ private void membersToDt(AggregateDeclaration ad, DtBuilder dtb,
        {
            dtb.xoff(toVtblSymbol(concreteType), 0);  // __vptr
            offset = Target.ptrsize;
-            if (!cd.classKind == ClassKind.cpp)
+            if (cd.classKind != ClassKind.cpp)


Should we look for more of these?

I did. I didn't find any. Feel free to check yourself.

jacob-carlborg · 2017-11-30T09:40:42Z

src/ddmd/dclass.d

@@ -242,6 +240,10 @@ extern (C++) class ClassDeclaration : AggregateDeclaration
    /// to prevent recursive attempts
    private bool inuse;

+    /// true if this class has an identifier, but was originally declared anonymous


Do we really need to mention the second part, after the comma?

jacob-carlborg · 2017-11-30T09:41:28Z

src/ddmd/dclass.d

@@ -242,6 +240,10 @@ extern (C++) class ClassDeclaration : AggregateDeclaration
    /// to prevent recursive attempts
    private bool inuse;

+    /// true if this class has an identifier, but was originally declared anonymous
+    /// used in support of https://issues.dlang.org/show_bug.cgi?id=17371
+    private bool isActuallyAnonymous;


the D convention is, as far as I know, to use the same name as the getter but prefix it with an underscore.

I haven't seen that. Show me where the precedent is established.

I don't know where/if it's written down somewhere. But I think the name is a bit strange anyway, having Actually as part of the name doesn't add anything.

The "Actually" documents the fact that we are lying, because the class has a __anonclass identifier. And remember I had to overload a method called isAnonymous, so that name is off limits.

And remember I had to overload a method called isAnonymous, so that name is off limits.

That's why it's common to name the field either _isAnonymous or isAnonymous_. I prefer the latter but I think the former is more common in Phobos.

That's why it's common to name the field either _isAnonymous or isAnonymous_

I would prefer something like that as well, but that doesn't seem to be the convention in DMD.

No, but in the rest of the D community, at least in the core projects, the ones in the dlang GitHub organization. Keep in mind that DMD is quite an old code base written in a very old style, C with classes. We're trying to improve that and in my opinion any new code should follow the standard D conventions. We don't want to make it worse by adding new code that is written in the old style 😃.

jacob-carlborg · 2017-11-30T09:41:49Z

The C++ header need to be updated as well.

JinShil · 2017-11-30T09:43:24Z

The C++ header need to be updated as well.

aggregate.h was updated. Are you referring to something else?

jacob-carlborg · 2017-11-30T09:45:01Z

aggregate.h was updated. Are you referring to something else?

No, but the new field was not added.

JinShil · 2017-11-30T09:45:50Z

No, but the new field was not added.

Which new field?

RazvanN7 · 2017-11-30T09:45:54Z

@jacob-carlborg It shouldn't, since it is private.

jacob-carlborg · 2017-11-30T09:46:13Z

isActuallyAnonymous.

jacob-carlborg · 2017-11-30T09:46:43Z

The sizes of the D and the C++ classes need to match, as far as I know.

JinShil · 2017-11-30T09:50:33Z

The sizes of the D and the C++ classes need to match, as far as I know.

None of the private fields are declared in aggregate.h::ClassDeclaration. Please be more specific, I'm getting confused. What precisely needs to be done?

jacob-carlborg · 2017-11-30T09:53:43Z

None of the private fields are declared in aggregate.h::ClassDeclaration

It was only one, inuse, before this new field, which was recently added. Which I think needs to be added as well. Currently, if the C++ code tries to access the isabstract field it will actually get the inuse field, if I understand how this works.

Please be more specific, I'm getting confused. What precisely needs to be done?

Please add the inuse and the isActuallyAnonymous fields to the C++ header aggregate.h::ClassDeclaration.

JinShil · 2017-11-30T10:03:30Z

Please add the inuse and the isActuallyAnonymous fields to the C++ header aggregate.h::ClassDeclaration.

Done. Thank you!

jacob-carlborg · 2017-11-30T10:04:05Z

Example:

$ cat foo.d
module foo;

struct Foo
{
    private int a;
    int b;
}

extern (C) Foo* getFoo()
{
    import core.stdc.stdlib;

    auto foo = cast(Foo*) malloc(Foo.sizeof);
    foo.a = 3;
    foo.b = 4;

    return foo;
}

$ dmd -c -betterC foo.d

$ cat main.c
#include <stdio.h>

struct Foo
{
    int b;
};

struct Foo *getFoo();

int main (int i, char** c)
{
    struct Foo *foo = getFoo();
    printf("%d\n", foo->b);
}

$ clang main.c -o main foo.o
$ ./main
3

The above prints 3 and not 4 because the sizes of the structs on the D side and the C side doesn't match. It's the same idea with the D class and the C++ class.

jacob-carlborg · 2017-11-30T10:05:35Z

Done. Thank you!

The C++ header file looks good now.

jacob-carlborg · 2017-11-30T10:09:17Z

BTW, if you want to, the fields can be private on the C++ side as well. Although I'm not sure if the inuse field is used by LDC/GDC.

JinShil · 2017-11-30T10:12:53Z

BTW, if you want to, the fields can be private on the C++ side as well.

I'm don't want to refactor anything; I just want to fix the regression. The others fields are public so the new fields are public. If we need to be specific with access modifiers for the C++ interface, I think it should be done in a different PR.

jacob-carlborg · 2017-11-30T10:14:03Z

I'm don't want to refactor anything; I just want to fix the regression

Fair enough.

jacob-carlborg · 2017-11-30T10:16:17Z

LGTM 👌.

JinShil · 2017-11-30T10:25:36Z

The only thing preventing me from pulling this is I'm not sure if it should target master or stable. The PR that introduced this regression was only merged yesterday, so I'm guessing it needs to go to master. Please advise.

jacob-carlborg · 2017-11-30T10:27:05Z

The only thing preventing me from pulling this is I'm not sure if it should target master or stable. The PR that introduced this regression was only merged yesterday, so I'm guessing it needs to go to master. Please advise.

@MartinNowak ^

JinShil · 2017-11-30T11:27:39Z

It must be master. I don't see the changes in stable yet. auto-merge toggling on.

MartinNowak · 2017-11-30T14:16:13Z

... but it can be ClassKind.d and anonymous at the same time, right?

In fact anonymous classes always seem to be ClassKind.d, at least I didn't manage to produce a non-D anon class. But it's better to make this explicit, instead of relying on the fact that sth. cannot be expressed in the language's grammar.

MartinNowak · 2017-11-30T14:19:22Z

The PR that introduced this regression was only merged yesterday, so I'm guessing it needs to go to master.

Hence [Reg 2.078] which is the next upcoming release, it means the fix needs to go into 2.078.0, which will be branched off master on Dec 15 2017, and gets released on Jan 1 2018.
Also see https://calendar.google.com/calendar/embed?src=ut29n9vq9vu3ad4kbaabeed1d8%40group.calendar.google.com.

jacob-carlborg · 2017-11-30T14:42:30Z

In fact anonymous classes always seem to be ClassKind.d, at least I didn't manage to produce a non-D anon class.

Yes, that's correct. It was originally put in the enum to enforce that a class in the AST should not be anonymous and Objective-C/C++ at the same time. Which was possible before and is now possible again.

MartinNowak · 2017-11-30T19:06:36Z

Which was possible before and is now possible again.

Well it's still not possible in the language and the code the more self-explaining now.

jacob-carlborg · 2017-11-30T19:25:27Z

the code the more self-explaining now

I disagree. Ideally the API should force you do to the correct things as much as possible. In this case none of the solutions are prefect.

MartinNowak · 2017-12-01T14:45:05Z

Could easily add an assertion that anonymous implies extern(D) atm.

ibuclaw · 2017-12-01T18:32:20Z

src/ddmd/dsymbol.d

@@ -269,7 +269,7 @@ extern (C++) class Dsymbol : RootObject
        return false;
    }

-    final bool isAnonymous()
+    bool isAnonymous()


So you made this function virtual, but didn't reflect it in c++ headers?

I thought I had the C++ headers covered but apparently not.

Fix C++ Interface after PR #7378

dlang-bot added the Bug Fix label Nov 30, 2017

WalterBright approved these changes Nov 30, 2017

View reviewed changes

RazvanN7 mentioned this pull request Nov 30, 2017

Fix Issue 17371 - [REG 2.074.0] di generation broken for anonymous classes #7315

Merged

RazvanN7 approved these changes Nov 30, 2017

View reviewed changes

jacob-carlborg reviewed Nov 30, 2017

View reviewed changes

Fix Issue 18020 - [Reg 2.078] no property opCmp for anon class

da7e071

JinShil added the auto-merge label Nov 30, 2017

dlang-bot merged commit 39981da into dlang:master Nov 30, 2017

JinShil deleted the fix_18020 branch November 30, 2017 11:59

ibuclaw reviewed Dec 1, 2017

View reviewed changes

JinShil mentioned this pull request Dec 2, 2017

Fix C++ Interface after PR #7378 #7385

Merged

WalterBright added a commit that referenced this pull request Dec 2, 2017

Merge pull request #7385 from JinShil/fix_18020_pt2

19c287f

Fix C++ Interface after PR #7378

Fix Issue 18020 - [Reg 2.078] no property opCmp for anon class #7378

Fix Issue 18020 - [Reg 2.078] no property opCmp for anon class #7378

Conversation

JinShil commented Nov 30, 2017 • edited

dlang-bot commented Nov 30, 2017 • edited

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

WalterBright left a comment

Choose a reason for hiding this comment

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

RazvanN7 left a comment

Choose a reason for hiding this comment

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil Nov 30, 2017 • edited

Choose a reason for hiding this comment

jacob-carlborg Nov 30, 2017 • edited

Choose a reason for hiding this comment

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

RazvanN7 commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017 • edited

JinShil commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017 • edited

JinShil commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

JinShil commented Nov 30, 2017

MartinNowak commented Nov 30, 2017

MartinNowak commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

MartinNowak commented Nov 30, 2017

jacob-carlborg commented Nov 30, 2017

MartinNowak commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil commented Nov 30, 2017 •

edited

dlang-bot commented Nov 30, 2017 •

edited

JinShil Nov 30, 2017 •

edited

jacob-carlborg Nov 30, 2017 •

edited

jacob-carlborg commented Nov 30, 2017 •

edited

jacob-carlborg commented Nov 30, 2017 •

edited