Skip to content

fix Issue 18905 - [Reg 2.079] C++ classes can no longer be used with … #8304

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.

Sign up for GitHub

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

Merged
dlang-bot merged 1 commit into from
May 29, 2018
Merged

fix Issue 18905 - [Reg 2.079] C++ classes can no longer be used with … #8304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/dmd/toobj.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,14 @@ void toObjFile(Dsymbol ds, bool multiobj)
return;
}

const bool gentypeinfo = global.params.useTypeInfo && Type.dtypeinfo;
Copy link
Contributor

@JinShil JinShil May 29, 2018

Choose a reason for hiding this comment

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

If you merge dlang/druntime#2184, then you'll only need to check Type.dtypeinfo because the runtime will no longer import the TypeInfo classes when compiling with -betterC. There are other places in the DMD source code that can be simplified due to that PR.

Copy link
Member Author

@WalterBright WalterBright May 29, 2018

Choose a reason for hiding this comment

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

I'm not so sure about that. 2184 seems to be more concerned with the nothrow issue, which is unrelated to this PR's fix. It isn't ready yet, either, and this PR fix works and is pretty simple.

Copy link
Contributor

@JinShil JinShil May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

2184 is actually more concerned with preventing the import of code which is not relevant to a -betterC build. The nothrow addition was added because -betterC code should be nothrow at all times. If you want me to separate the two into different PRs, I can. Just let me know. And, it is all green and ready to go.

Copy link
Member Author

@WalterBright WalterBright May 29, 2018

Choose a reason for hiding this comment

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

I'm not too happy about the nothrow business, as I mentioned. But we should discuss that elsewhere, not here. I also do not believe that 2184 will resolve this issue, as note it took two different tests to get it through the test suite, not just checking typeinfo.

Copy link
Contributor

@JinShil JinShil May 29, 2018

Choose a reason for hiding this comment

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

Understood. Please see dlang/druntime#2194. It prevents unnecessary imports in -betterC, and removes the nothrow issue.

const bool genclassinfo = gentypeinfo || !(cd.isCPPclass || cd.isCOMclass);

// Generate C symbols
toSymbol(cd);
toVtblSymbol(cd);
Symbol *sinit = toInitializer(cd);
if (genclassinfo)
toSymbol(cd); // __ClassZ symbol
toVtblSymbol(cd); // __vtblZ symbol
Symbol *sinit = toInitializer(cd); // __initZ symbol

//////////////////////////////////////////////

Expand All @@ -376,16 +380,18 @@ void toObjFile(Dsymbol ds, bool multiobj)
outdata(sinit);
}

if (genclassinfo)
{
//////////////////////////////////////////////

// Put out the TypeInfo
if (global.params.useTypeInfo && Type.dtypeinfo)
if (gentypeinfo)
genTypeInfo(cd.loc, cd.type, null);
//toObjFile(cd.type.vtinfo, multiobj);

//////////////////////////////////////////////

// Put out the ClassInfo
// Put out the ClassInfo, which will be the __ClassZ symbol in the object file
cd.csym.Sclass = scclass;
cd.csym.Sfl = FLdata;

Expand Down Expand Up @@ -494,6 +500,9 @@ void toObjFile(Dsymbol ds, bool multiobj)
}
if (cd.isAbstract())
flags |= ClassFlags.isAbstract;

flags |= ClassFlags.noPointers; // initially assume no pointers
Louter:
for (ClassDeclaration pc = cd; pc; pc = pc.baseClass)
{
if (pc.members)
Expand All @@ -503,12 +512,13 @@ void toObjFile(Dsymbol ds, bool multiobj)
Dsymbol sm = (*pc.members)[i];
//printf("sm = %s %s\n", sm.kind(), sm.toChars());
if (sm.hasPointers())
goto L2;
{
flags &= ~ClassFlags.noPointers; // not no-how, not no-way
break Louter;
}
}
}
}
flags |= ClassFlags.noPointers;
L2:
Copy link
Member

@MartinNowak MartinNowak Jun 6, 2018

Choose a reason for hiding this comment

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

Please avoid seemingly unrelated refactorings in bugfixes or mention why they were necessary.

dtb.size(flags);

// deallocator
Expand Down Expand Up @@ -609,6 +619,7 @@ void toObjFile(Dsymbol ds, bool multiobj)
outdata(cd.csym);
if (cd.isExport())
objmod.export_symbol(cd.csym, 0);
}

//////////////////////////////////////////////

Expand Down
6 changes: 6 additions & 0 deletions test/compilable/test18905.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* REQUIRED_ARGS: -betterC
*/

// https://issues.dlang.org/show_bug.cgi?id=18905
Copy link
Contributor

@JinShil JinShil May 29, 2018

Choose a reason for hiding this comment

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

Don't you need -betterC compiler flags here? Also, you might want to add this to the existing betterc.d test file.

Copy link
Member Author

@WalterBright WalterBright May 29, 2018

Choose a reason for hiding this comment

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

Yes, good catch!


extern (C++) class C { } // Error: TypeInfo cannot be used with -betterC