[stable] Fix issue 20700 - Forward references leads to extern(C++, class|struct) being ignored #10988
Conversation
|
Thanks for your pull request, @Geod24! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "stable + dmd#10988" |
Geod24
force-pushed
the
fix-20700
branch
3 times, most recently
from
d96ee5e
to
e05468b
Compare
Geod24
force-pushed
the
fix-20700
branch
2 times, most recently
from
af6b48e
to
c533ad1
Compare
|
Found out the issue is harder than expected so back tip |
Geod24
force-pushed
the
fix-20700
branch
5 times, most recently
from
23ad49c
to
350ee80
Compare
Geod24
requested review from
andralex,
CyberShadow,
jacob-carlborg,
MartinNowak,
rainers,
RazvanN7,
WalterBright and
wilzbach
as code owners
| buf.writeByte('T'); | ||
| else | ||
| buf.writeByte(type.cppmangle == CPPMANGLE.asClass ? 'V' : 'U'); | ||
| mangleIdent(type.sym); | ||
| buf.writeByte(agg.cppmangle == CPPMANGLE.asClass ? 'V' : 'U'); |
There was a problem hiding this comment.
.cppmangle is a field in AggregateDeclaration, but the only use of agg is to get that field. Shouldn't agg be an AggregateDeclaration, not a StructDeclaration ?
There was a problem hiding this comment.
It could be, but since we are in visit(TypeStruct), we know that it's a StructDeclaration already, and I thought it'd be less confusing.
Geod24
force-pushed
the
fix-20700
branch
2 times, most recently
from
bc3fdfe
to
4f4b97c
Compare
| @@ -82,6 +82,10 @@ extern (C++) abstract class AggregateDeclaration : ScopeDsymbol | |||
|
|
|||
| /// specifies whether this is a D, C++, Objective-C or anonymous struct/class/interface | |||
| ClassKind classKind; | |||
| /// Specify whether to mangle the aggregate as a `class` or a `struct` | |||
| /// This information is used by the MSVC mangler | |||
| /// Only valid for class and struct. TODO: Merge with ClassKind ? | |||
There was a problem hiding this comment.
Yes. It looks like it should be merged with classKind.
There was a problem hiding this comment.
I'm not doing it here just because this targets stable
…gnored `Type`-derived AST nodes do not support the same kind of semantic analysis that `Dsymbol`-derived one do. The issue was that in some codes, semantic analysis on the type was performed before it was performed on the declaration, and chasing those cases turned out to be endless, hence the minor refactoring.
| extern(C++, class) struct TStruct20700_1 (T1, T2 = DefaultStruct20700_1) {} | ||
| extern(C++, class) struct DefaultStruct20700_1 {} | ||
| extern(C++, struct) class DefaultClass20700_1 {} | ||
| static assert(test20700_3.mangleof == `?test20700_3@@YAXV?$TStruct20700_1@PEAUDefaultClass20700_1@@VDefaultStruct20700_1@@@@@Z`); |
There was a problem hiding this comment.
This is demangled by UnDecorateSymbolName to
void __cdecl test20700_3(class TStruct20700_1<struct DefaultClass20700_1 * __ptr64,class DefaultStruct20700_1>)
so it seems class and struct are swapped.
There was a problem hiding this comment.
Ah, sorry for the noise, I somehow missed that you explicitly changed the mangling.
There was a problem hiding this comment.
DefaultStruct is a D struct but mangled as a class. DefaultClass is a D class but mangled as a struct (well, a pointer to struct). Same for TStruct..., it's a D struct, but mangled as class.
|
Ready to go (@thewilsonator ) |
Cleaning up after dlang#10952 and dlang#10988.
Note: This is a WIP for the moment as I can't test on Windows. However, I'd like to be sure the test fails before I make it pass.