Refactor mangle-only `extern(C++, namespace)` into an AttribDeclaration #10021

Geod24 · 2019-06-12T05:32:58Z

Originally, `extern(C++, ident)` was implemented as introducing a scope,
importing C++ semantics into D, and was made a `ScopeDsymbol`.
Introducing a scope turned out to be a behavior that was problematic,
and users of `extern(C++)` just wanted to use D semantics and have the correct mangling.

Later on, `extern(C++, "namespace")` was implemented, which removed the introduction of a scope.
However, it was implemented within the existing `Nspace`,
basically hacking around the fact that `ScopeDsymbol` introduces a scope
and injecting the symbols in the parent scope.

This commit implements a new AST node, `CPPNamespaceDeclaration`, which is an `AttributeDeclaration`.
`AttributeDeclaration` are the way storage classes (e.g. `extern(C)` and `extern(C++, class|struct)` are implemented,
as well as declarations that apply to a scope without introducing one, such as `DeprecatedDeclaration`.

Since mangling is part of the function's external interface,
CTFE on the message is run during the first semantic pass.
Since `_scope` is set earlier than `semantic` is done on the attribute,
the full `CPPNamespaceDeclaration` needs to be stored in the `Scope`,
not just the resolved `Identifiers`.

Moreover, a few tweaks are required to `cppmangle` to make it work with the new AST structure.
Once the scoped version of `extern(C++, namespace)` is deprecated, the code can be greatly simplified.

Over the past two weeks I tried two things:

To fix Fix issue 19893 - extern(C++ 'ns') should count as module scope #9912 (comment)
To deprecate the scoped version of extern(C++, namespace)

After much trial and error, I came up with the explanation mentioned above.
This will fail the CI for now as I haven't tried it on Windows yet.
I can split a few changes in separate PRs, but the bulk of it is indissociable.

CC @TurkeyMan

dlang-bot · 2019-06-12T05:32:59Z

Thanks for your pull request, @Geod24!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10021"

thewilsonator · 2019-06-13T13:56:51Z

Can you split the first three commits as a separate PR please.

Geod24 · 2019-06-13T14:21:39Z

Can you split the first three commits as a separate PR please.

Sure thing: #10032

Geod24 · 2019-06-13T23:55:28Z

Since this pass the full extern(C++) test-suite (as #10031 converts all the extern(C++, ns) into extern(C++, "ns") I think this is ready.

Originally, `extern(C++, ident)` was implemented as introducing a scope, importing C++ semantics into D, and was made a `ScopeDsymbol`. Introducing a scope turned out to be a behavior that was problematic, and users of `extern(C++)` just wanted to use D semantics and have the correct mangling. Later on, `extern(C++, "namespace")` was implemented, which removed the introduction of a scope. However, it was implemented within the existing `Nspace`, basically hacking around the fact that `ScopeDsymbol` introduces a scope and injecting the symbols in the parent scope. This commit implements a new AST node, `CPPNamespaceDeclaration`, which is an `AttributeDeclaration`. `AttributeDeclaration` are the way storage classes (e.g. `extern(C)` and `extern(C++, class|struct)` are implemented, as well as declarations that apply to a scope without introducing one, such as `DeprecatedDeclaration`. Since mangling is part of the function's external interface, CTFE on the message is run during the first semantic pass. Since `_scope` is set earlier than `semantic` is done on the attribute, the full `CPPNamespaceDeclaration` needs to be stored in the `Scope`, not just the resolved `Identifiers`. Moreover, a few tweaks are required to `cppmangle` to make it work with the new AST structure. Once the scoped version of `extern(C++, namespace)` is deprecated, the code can be greatly simplified.

Geod24 · 2019-06-14T01:32:57Z

Rebased on master since #10032 got merged

src/dmd/attrib.d

src/dmd/attrib.h

src/dmd/cppmangle.d

WalterBright · 2019-06-15T01:29:43Z

Why is this called "refactoring" when @SSoulaimane claims it introduces a scope? Where does it say that? Where's the documentation?

SSoulaimane · 2019-06-15T01:32:33Z

Why is this called "refactoring" when @SSoulaimane claims it introduces a scope? Where does it say that? Where's the documentation?

Sorry, I meant this fixed the 'introduce a scope then discard it" i.e. no scope anymore. It's just a AttribDeclaration.

WalterBright · 2019-06-15T01:34:26Z

I surely do not understand how the code is better with this refactoring.

Geod24 · 2019-06-15T13:12:26Z

#10037 (comment)

SSoulaimane · 2019-06-15T18:15:10Z

src/dmd/cppmangle.d

+            buf.writestring("N");
+            if (!this.substitute(sym2))
+                sym2.accept(this);
+            dg();


@Geod24 duplicated call to dg(). and missing terminator "E" for "N".
It seems this branch isn't covered in the tests.

extern(C++, A) struct S(T){} extern(C++) void fun(int i)(S!int); pragma(msg, fun!0.mangleof);

Prints _Z3funILi0EEvN1AIiE1SIiE. should be _Z3funILi0EEvN1A1SIiEE.

SSoulaimane · 2019-06-15T18:15:21Z

src/dmd/cppmangle.d

+                sym2.accept(this);
+            dg();
+        }
+        if (sym1 !is null)


redundant check. sym1 in never null here.

SSoulaimane · 2019-06-15T19:30:02Z

src/dmd/dsymbolsem.d

+            const sident = se.toStringz();
+            if (!sident.length || !Identifier.isValidIdentifier(sident))
+            {
+                ns.exp.error("expected valid identifer for C++ namespace but got `%.*s`",


Wasn't the motive of this syntax that D identifiers don't exactly match C++? 15512.

Yes, it's one of the motives, and very important.

Then checking the validity here defeats the point.

This code was adapted from what's existing. I also had a "Wait, what?" moment until I checked the code for isValidIdentifier. It does not check if the identifier is one of the reserved ones.

cppmangle: Follow up on #10021 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

SSoulaimane · 2019-06-16T18:16:42Z

src/dmd/attrib.d

+
+    extern(D) override const(char)[] toString() const
+    {
+        return "extern (C++, `namespace`)";


Should include the identifier or expression instead of "namespace".

Nspace doesn't do that (and it allocates)
I was considering doing a refactor where toString takes a sink, but it has been put off so far.

SSoulaimane · 2019-06-16T18:43:00Z

src/dmd/cppmangle.d

+                    s.namespace, () {
+                        this.writeIdentifier(ti.tempdecl.toAlias().ident);
+                        template_args(ti);
+                    });


source_name isn't supposed to insert N..E but writeNamespace does.

extern(C++, "a", "b") struct S { void func(); } pragma(msg, S.func.mangleof);

This is a regression. It prints _ZNN1a1b1SE4funcEv it used to be _ZN1a1b1S4funcEv.

ibuclaw · 2019-06-19T23:17:04Z

src/dmd/dsymbol.d

-            return null;
-        return parent.pastMixinAndNspace();
-    }
+    alias pastMixinAndNspace = pastMixin;


This caused a regression (C++ headers weren't updated).

ibuclaw · 2019-06-19T23:19:36Z

src/dmd/dsymbol.d

@@ -1187,6 +1180,7 @@ extern (C++) class Dsymbol : ASTNode
    inout(SymbolDeclaration)           isSymbolDeclaration()           inout { return null; }
    inout(AttribDeclaration)           isAttribDeclaration()           inout { return null; }
    inout(AnonDeclaration)             isAnonDeclaration()             inout { return null; }
+    inout(CPPNamespaceDeclaration)     isCPPNamespaceDeclaration()     inout { return null; }


And another regression...

Geod24 · 2019-06-29T14:16:50Z

This also fixed https://issues.dlang.org/show_bug.cgi?id=19339

RazvanN7 · 2024-06-04T13:45:07Z

This seems to have caused a regression: https://issues.dlang.org/show_bug.cgi?id=24547

Geod24 requested review from ibuclaw and RazvanN7 as code owners June 12, 2019 05:32

Geod24 force-pushed the refactor-extern-cpp branch 12 times, most recently from 93cd57c to 28ab07a Compare June 12, 2019 23:54

Geod24 mentioned this pull request Jun 13, 2019

Fix issue 19949 - C++ Mangling: add support for abi-tags from the Itanium ABI #9995

Closed

Geod24 force-pushed the refactor-extern-cpp branch 3 times, most recently from 1e6697b to 40ee34c Compare June 13, 2019 12:10

Geod24 mentioned this pull request Jun 13, 2019

Deprecate extern(C++, identifier) #10031

Closed

Geod24 force-pushed the refactor-extern-cpp branch from 40ee34c to 375324a Compare June 13, 2019 14:17

Geod24 mentioned this pull request Jun 13, 2019

Minor C++ refactoring / bug fix #10032

Merged

Geod24 force-pushed the refactor-extern-cpp branch from 375324a to 3ff20df Compare June 13, 2019 17:35

Geod24 force-pushed the refactor-extern-cpp branch from 3ff20df to fd8a824 Compare June 14, 2019 01:31

thewilsonator approved these changes Jun 14, 2019

View reviewed changes

thewilsonator added the Merge:auto-merge label Jun 14, 2019

dlang-bot merged commit 140937d into dlang:master Jun 14, 2019

Geod24 deleted the refactor-extern-cpp branch June 14, 2019 19:09

WalterBright reviewed Jun 15, 2019

View reviewed changes

src/dmd/attrib.d Show resolved Hide resolved

WalterBright reviewed Jun 15, 2019

View reviewed changes

src/dmd/attrib.h Show resolved Hide resolved

WalterBright reviewed Jun 15, 2019

View reviewed changes

src/dmd/cppmangle.d Show resolved Hide resolved

WalterBright mentioned this pull request Jun 15, 2019

Revert "Refactor mangle-only extern(C++, namespace) into an AttribDeclaration" #10037

Closed

SSoulaimane reviewed Jun 15, 2019

View reviewed changes

src/dmd/cppmangle.d

sym2.accept(this);

dg();

}

if (sym1 !is null)

Copy link

Member

SSoulaimane Jun 15, 2019

Choose a reason for hiding this comment

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

redundant check. sym1 in never null here.

SSoulaimane reviewed Jun 15, 2019

View reviewed changes

dlang-bot added a commit that referenced this pull request Jun 16, 2019

Merge pull request #10044 from Geod24/follow-up-10021

ea1afd0

cppmangle: Follow up on #10021 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

SSoulaimane reviewed Jun 16, 2019

View reviewed changes

This was referenced Jun 18, 2019

cppmangle: Add more tests, fix double 'N' #10053

Merged

[stable] Fix extern(C++, "namespace") following refactor #10058

Merged

ibuclaw reviewed Jun 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor mangle-only `extern(C++, namespace)` into an AttribDeclaration #10021

Refactor mangle-only `extern(C++, namespace)` into an AttribDeclaration #10021

Geod24 commented Jun 12, 2019

dlang-bot commented Jun 12, 2019

thewilsonator commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Jun 14, 2019

WalterBright commented Jun 15, 2019

SSoulaimane commented Jun 15, 2019

WalterBright commented Jun 15, 2019

Geod24 commented Jun 15, 2019

SSoulaimane Jun 15, 2019 •

edited

Loading

SSoulaimane Jun 15, 2019

SSoulaimane Jun 15, 2019

TurkeyMan Jun 15, 2019 •

edited

Loading

SSoulaimane Jun 15, 2019

Geod24 Jun 16, 2019

SSoulaimane Jun 16, 2019

Geod24 Jun 17, 2019

SSoulaimane Jun 16, 2019

ibuclaw Jun 19, 2019

ibuclaw Jun 19, 2019

Geod24 commented Jun 29, 2019

RazvanN7 commented Jun 4, 2024

Refactor mangle-only extern(C++, namespace) into an AttribDeclaration #10021

Refactor mangle-only extern(C++, namespace) into an AttribDeclaration #10021

Conversation

Geod24 commented Jun 12, 2019

dlang-bot commented Jun 12, 2019

Bugzilla references

Testing this PR locally

thewilsonator commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Jun 13, 2019

Geod24 commented Jun 14, 2019

WalterBright commented Jun 15, 2019

SSoulaimane commented Jun 15, 2019

WalterBright commented Jun 15, 2019

Geod24 commented Jun 15, 2019

SSoulaimane Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TurkeyMan Jun 15, 2019 • edited Loading

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

Geod24 commented Jun 29, 2019

RazvanN7 commented Jun 4, 2024

Refactor mangle-only `extern(C++, namespace)` into an AttribDeclaration #10021

Refactor mangle-only `extern(C++, namespace)` into an AttribDeclaration #10021

SSoulaimane Jun 15, 2019 •

edited

Loading

TurkeyMan Jun 15, 2019 •

edited

Loading