DIP61: C++ namespaces #3517

WalterBright · 2014-05-01T09:58:40Z

Also started changing the license to Boost.

http://wiki.dlang.org/DIP61

yebblies · 2014-05-01T11:19:49Z

Boost stuff should be in another pull request.

dnadlinger · 2014-05-01T11:26:25Z

I'm also not sure whether you can just change the license without explicitly getting the okay from all other contributors. I'm not an expert for US copyright law, but usually, contributing code to an open source project under a certain license isn't necessarily seen as implying a copyright assignment.

WalterBright · 2014-05-01T17:52:28Z

isn't necessarily seen as implying a copyright assignment.

The files are copyrighted by Digital Mars, hence Digital Mars can change the license.

dnadlinger · 2014-05-01T18:26:47Z

@WalterBright: I don't think publishing a patch to a file with a copyright notice (e.g. opening a pull request) automatically implies assigning the copyright for the changes to the entity specified there. At least, people seem to doubt that such an opinion would hold up in court. Otherwise, there would be no reason for the various open source projects which require contributors to explicitly assign the copyright to do so.

WalterBright · 2014-05-01T18:35:58Z

The assigning copyright thing happens when somebody tries to submit copyright Me stuff to an open source project, the project requires them to make it copyright Them. This is why all the front end is copyrighted by Digital Mars, and when a couple people made contributions to it that they claimed personal copyright on, I asked them to change it before I'd accept the pull (and they did so).

WalterBright · 2014-05-01T18:39:34Z

Anyhow, I changed the license for cppmangle.c back for the moment because this discussion should be about DIP61, not licenses.

ghost · 2014-05-01T18:47:37Z

src/parse.c

+        {
+            assert(link != LINKdefault);
+            assert(idents->dim);
+            for (size_t i = idents->dim; i;)


Why not put --i in here?

It would be i--, not --i, and that confusion is why.

jacob-carlborg · 2014-05-01T18:56:51Z

Shouldn't there be tests for the actual mangling and linking with some C++ code?

WalterBright · 2014-05-01T19:01:04Z

Shouldn't there be tests for the actual mangling and linking with some C++ code?

Yes, and there are: see the test case cppa.d and cppb.cpp. The mangling is correct if the object files link successfully. No need to compare strings.

dnadlinger · 2014-05-01T19:46:23Z

@WalterBright: I agree that this should be discussed in a different place. However, my point was just that the fact that a patch does not change the copyright notice in a file might not necessarily be enough to assume that contributors assign their copyright to you. I am not a lawyer, and additionally our copyright system (Western Europe) is quite different from the U.S. one, so my understanding of the issue is likely not to be fully correct.

Still, many other open source projects make a much bigger deal out of this. It might just be a case of "better safe than sorry" for these projects, but if you know of a publicly available source supporting your opinion (of course, no legal advice, etc.), I'd be very interested.

WalterBright · 2014-05-01T20:38:02Z

@klickverbot let's discuss it in a different place, then. I'll do another PR just for that. And no, I don't have a document link. #3519

jacob-carlborg · 2014-05-01T20:54:45Z

Yes, and there are: see the test case cppa.d and cppb.cpp. The mangling is correct if the object files link successfully. No need to compare strings.

Now I see it.

ibuclaw · 2014-05-04T21:04:08Z

Hmm... this'll complicate areas around debug codegen.

WalterBright · 2014-05-04T21:40:33Z

How so? Why would it be any different than for extern (C) ?

ibuclaw · 2014-05-05T08:24:46Z

In terms of representing namespaces in debug. It's mostly an implementation restriction on my side.
In two sentences, namespace == module in AST. Namespace is emitted as module (DWARF) depending on language being compiled.

I also see in your examples that extern (C++, NS) implies using too. So that would also affect my gdb patches to handle proper symbol lookup in D (I'd have to maintain C++ compatibility, which might slow things down :o)

WalterBright · 2014-05-05T16:31:13Z

Shouldn't namespaces be emitted to DWARF the same way as C++ namespaces?

WalterBright · 2014-05-05T17:48:56Z

Documentation pull: dlang/dlang.org#572

andralex · 2014-05-13T01:07:59Z

src/nspace.c

+    if (members)
+    {
+        for (size_t i = 0; i < members->dim; i++)
+        {   Dsymbol *s = (*members)[i];


there should be no code after {, sigh

andralex · 2014-05-13T01:19:23Z

LGTM. I think the design is adequate.

yebblies · 2014-05-13T05:14:52Z

I have some problems with this, please give me some time to test it before you merge it.

WalterBright · 2014-05-16T00:06:58Z

@yebblies any update?

yebblies · 2014-05-16T09:54:07Z

@yebblies any update?

I don't think this deserves it's own syntax, ast class or new lookup rule. Here is a partial implementation using a pragma that should demonstrate how much simpler it could be.

I don't think adding new syntax for interfacing to other language features is a scalable approach, and the presence of namespace scopes in D will result in them being used for purposes other than interfacing with C++. We have a module system for that purpose and we shouldn't be introducing alternative organisation methods as a side effect of a change like this.

On the implementation side, will this produce garbage when a namespace is declared inside a class or other aggregate?

eg

class Foo
{
extern(C++, A.B.C) void func(); // Is this a valid construct?
}

I can't see a check to limit this feature to module scope, but maybe I missed it.

WalterBright · 2014-05-16T18:15:25Z

On the implementation side, will this produce garbage when a namespace is declared inside a class or other aggregate?

It's valid and works fine producing a function mangled as: ?func@C@B@A@Foo@@YAXXZ

WalterBright · 2014-05-16T18:24:36Z

. Here is a partial implementation using a pragma

I appreciate that you wrote an implementation proof of concept for your idea.

The downsides of it are, as I see it:

it's fairly wordy for what it does
the pragma doesn't have an obvious syntactical connection with extern (C++)
I don't see any provision in it for dealing with the same name in multiple namespaces, i.e. how the caller is to disambiguate which one he means
I suspect issues with inadvertent overloading of a name appearing in multiple namespaces

I also believe we discussed this at length in the n.g. I wish we could have arrived at a consensus, but most people did indicate a preference for the PR's design.

ibuclaw · 2014-05-17T09:01:36Z

Shouldn't namespaces be emitted to DWARF the same way as C++ namespaces?

Yes, the problem is an implementation detail left and right.

Left because I don't think GCC anticipated a language to emit both DWARF modules and DWARF namespaces.

Right because I never anticipated D supporting namespaces (in the patches I've been prepp'ing and sending to GDB).

yebblies · 2014-05-17T17:00:54Z

I appreciate that you wrote an implementation proof of concept for your idea.

I don't like arguing against a pull request for an alternative feature without having one of my own. An opinion with a patch carries a lot more weight in my view,

The downsides of it are, as I see it:

it's fairly wordy for what it does

I don't think this is a big problem, because it should be very uncommon feature in user code. It is a low-level interfacing feature, and I seriously hope most uses will be in (semi-)automatically generated bindings. My experience is most developers would not be able to easily fix the linker errors caused by using this incorrectly.

the pragma doesn't have an obvious syntactical connection with extern (C++)

This is intentional. extern(...) currently changes the mangling scheme and ABI. You are adding a full namespacing feature to D with this pull request, and hiding it in a syntax that makes it convenient only for interfacing with C++.

eg This code will be valid D, and it WILL be used like this (it will replace the current struct/class workarounds).

extern(C++, MyNamespace)
{
extern(D):
    ...
}

I'm sure you can agree that we can come up with better syntax for namespaces if we want to add them as first-class constructs.

Since you're effectively adding full namespaces, why should they syntactically be tied to extern?

I don't see any provision in it for dealing with the same name in multiple namespaces, i.e. how the caller is to disambiguate which one he means

Functions with the same name/arguments should be placed in different D modules. The module is D's unit of organisation and provides powerful tools for disambiguation.

If this is not sufficient, then we should consider adding namespaces to D with a dedicated syntax.

I suspect issues with inadvertent overloading of a name appearing in multiple namespaces

I can hardly argue that you don't suspect something. Can you think of any issues?

I also believe we discussed this at length in the n.g. I wish we could have arrived at a consensus, but most people did indicate a preference for the PR's design.

As I recall most of the objections to my objections were actually misunderstandings. Hopefully now you have a better understanding of what I'm proposing and why.

In summary - this pull adds namespaces to D, and ties them syntactically to extern(C++).
I think we should instead do one of:

Only add C++ namespace mangling (via pragma(cpp_namespace) or equivalent)
Add namespaces with their own syntax.

jacob-carlborg · 2014-05-17T19:31:34Z

Only add C++ namespace mangling (via pragma(cpp_namespace) or equivalent)

Add namespaces with their own syntax.

I would prefer the first option.

mihails-strasuns · 2014-05-18T00:37:10Z

Same here.

WalterBright · 2014-05-18T18:27:51Z

This was all hashed out here:

http://forum.dlang.org/thread/ljjnaa$187r$1@digitalmars.com

andralex · 2014-05-18T18:44:03Z

I think this proposal is plenty adequate, and in my opinion it's better than @yebblies' alternative. Both designs "work", so at some point a judgment call has to be made. I'll pull this now. We can unpull later if a good argument comes about.

DIP61: C++ namespaces

ibuclaw · 2014-05-18T20:01:35Z

FYI:

Standard special-case substitutions in C++ (for Itanium)

  <substitution> ::= St
      # ::std

  <substitution> ::= Sa
      # ::std::allocator

  <substitution> ::= Sb
      # ::std::basic_string

  <substitution> ::= Ss
      # ::std::basic_string<char, ::std::char_traits<char>, ::std::allocator<char> >

  <substitution> ::= Si
      # ::std::basic_istream<char, ::std::char_traits<char> >

  <substitution> ::= So
      # ::std::basic_ostream<char, ::std::char_traits<char> >

  <substitution> ::= Sd
      # ::std::basic_iostream<char, ::std::char_traits<char> >

The first three are probably all that matter. The others only apply to specific C++ templates.

ibuclaw · 2014-05-18T20:13:49Z

This is just incase someone thinks they can now do extern (C++, std) { ... } and things will link just fine.

JesseKPhillips · 2014-06-24T02:14:34Z

FYI: Resolved #7961

Add informative range errors for arrays Signed-off-by: Nicholas Wilson <thewilsonator@users.noreply.github.com> Merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

WalterBright changed the title ~~DIP69: C++ namespaces~~ DIP61: C++ namespaces May 1, 2014

ghost reviewed May 1, 2014
View reviewed changes

WalterBright mentioned this pull request May 4, 2014

Add license to visitor header #3523

Merged

WalterBright mentioned this pull request May 5, 2014

Document C++ namespace support dlang/dlang.org#572

Merged

andralex reviewed May 13, 2014
View reviewed changes

DIP69: C++ namespaces

1678b09

andralex added a commit that referenced this pull request May 18, 2014

Merge pull request #3517 from WalterBright/DIP69

b66034f

DIP61: C++ namespaces

andralex merged commit b66034f into dlang:master May 18, 2014

WalterBright deleted the DIP69 branch May 18, 2014 18:46

DIP61: C++ namespaces #3517

DIP61: C++ namespaces #3517

Conversation

WalterBright commented May 1, 2014

yebblies commented May 1, 2014

dnadlinger commented May 1, 2014

WalterBright commented May 1, 2014

dnadlinger commented May 1, 2014

WalterBright commented May 1, 2014

WalterBright commented May 1, 2014

ghost May 1, 2014

Choose a reason for hiding this comment

WalterBright May 1, 2014

Choose a reason for hiding this comment

jacob-carlborg commented May 1, 2014

WalterBright commented May 1, 2014

dnadlinger commented May 1, 2014

WalterBright commented May 1, 2014

jacob-carlborg commented May 1, 2014

ibuclaw commented May 4, 2014

WalterBright commented May 4, 2014

ibuclaw commented May 5, 2014

WalterBright commented May 5, 2014

WalterBright commented May 5, 2014

andralex May 13, 2014

Choose a reason for hiding this comment

WalterBright May 13, 2014

Choose a reason for hiding this comment

andralex commented May 13, 2014

yebblies commented May 13, 2014

WalterBright commented May 16, 2014

yebblies commented May 16, 2014

WalterBright commented May 16, 2014

WalterBright commented May 16, 2014

ibuclaw commented May 17, 2014

yebblies commented May 17, 2014

jacob-carlborg commented May 17, 2014

mihails-strasuns commented May 18, 2014

WalterBright commented May 18, 2014

andralex commented May 18, 2014

ibuclaw commented May 18, 2014

ibuclaw commented May 18, 2014

JesseKPhillips commented Jun 24, 2014