Added std::array<> #2286
Added std::array<> #2286
Conversation
|
Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour 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 locallyIf 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 + druntime#2286" |
TurkeyMan
requested review from
andralex,
WalterBright and
wilzbach
as code owners
| @@ -46,6 +46,8 @@ SRCS=\ | |||
| src\core\stdc\time.d \ | |||
| src\core\stdc\wchar_.d \ | |||
| \ | |||
| src\core\stdcpp\array.d \ | |||
There was a problem hiding this comment.
Why aren't any other the other existing files in here?
src/core/stdcpp/array.d
Outdated
|
|
||
| alias as_array this; | ||
|
|
||
| extern(D) size_type size() const nothrow @safe @nogc { return N; } |
There was a problem hiding this comment.
You can just stick an extern(D): at the top. Also I would explain in the doc why those are extern(D), as it might be confusing to users/reviewers.
There was a problem hiding this comment.
Yeah, I thought about that... but then I thought it might be too easy to accidentally mis-attribute any proper extern's.
There was a problem hiding this comment.
Any proper extern(C++) will have it in front of the prototype, won't it ?
I remember doing this for a project of my own and not having to define any extern(C++) anyway because everything gets inlined.
There was a problem hiding this comment.
Yes, . Looking at this on my phone, didn’t scroll to the code right.extern(D) is confusing to me.
There was a problem hiding this comment.
If these functions are extern(C++), they may conflict with actual C++ implementations of these functions. Locally defined functions should be D-mangled.
But what does it matter, you guys are rejecting it all anyway :/
|
|
||
| extern(C++, std): | ||
|
|
||
| extern(C++, class) struct array(T, size_t N) |
src/core/stdcpp/array.d
Outdated
|
|
||
| module core.stdcpp.array; | ||
|
|
||
| alias array = std.array; |
There was a problem hiding this comment.
https://forum.dlang.org/thread/[email protected]
Knock yourself out!
There was a problem hiding this comment.
TL;DR, Walter says.
There was a problem hiding this comment.
TL;DR, slightly longer: you can't import 2 such modules because the namespace scopes conflict with eachother. Also, the namespace called 'std' conflicts with phobos. Basically, we just need to alias all these things out into the module scope (where they should be anyway) so that anything is accessible.
There was a problem hiding this comment.
Makes sense.
Can you add a comment (not DDOC) to mention this ?
src/core/stdcpp/array.d
Outdated
| /** | ||
| * D header file for interaction with C++ std::array. | ||
| * | ||
| * Copyright: Manu Evans 2014 - 2018. |
There was a problem hiding this comment.
True.. what's the proper copyright assignment?
I just copied what I found in neighbouring code.
|
Can you add ABI tests ? |
TurkeyMan
force-pushed
the
std_array
branch
2 times, most recently
from
caeb696 to
84c614b
Compare
|
I don't really know how to test this. It involves interaction with all the possible C compilers, and the build/CI scripts are chaos. |
|
Why are we putting C++ bindings in druntime? Can't these exist in a Dub package that doesn't require the D Language Foundation to take ownership of it, and assume the burden of maintaining it? |
|
This was a D Language Foundation project from a while back. There was a hired employee working on this. I guess it feels natural that they exist beside There's no way a dub package will maintain correct sync, and these files, possibly more than any other files in the distribution, definitely need to run their CI and tests together with the releases to guarantee ABI match. |
|
Anything less will be chaos. These bindings are of critical importance, and really need to 'just work'. An external repo will never keep sync. I don't think they can live anywhere else. |
|
I realize someone decided long ago that they should be here in druntime; I'm challenging that decision. I'm not convinced that a Dub repository would not work. Even if the D Language Foundation chose to take ownership of these bindings, they could exist in a different repository or https://github.com/D-Programming-Deimos/ I think the only reason why the C standard library bindings are here is because they are used by some of the druntime internals. If druntime has no need for these bindings internally, I don't think they belong here. Even if druntime did use them internally, there are other ways to go about that. I ask that we strive to keep the scope of druntime to be just what is necessary to implement the D language, and nothing more. |
|
These files very closely track the build scripts, any C compilers that the toolchain builds call out to need to be correctly reflected here, also C++ related stuff is very hot in DMD, and these should also be checked against every C++ related change to DMD. I understand your argument, and for anything else, I'd agree, but this is intimately linked to the build process of the toolchain. It can't float around on its own. |
|
They're also not like other 'libraries', these aren't subject to active development. They're not 'hot', they'll never change once they're correct, so it doesn't suffer dev characteristics like in-development phobos modules that are being spun up in dub packages. |
|
C++ is definitely not a 'deimos' binding. This isn't 'just some lib' we're making available to D users for convenience... as long as Consider GDC updates to the next version of GCC, or we upgrade the win builds to VS2019, we need tests to fail, and to correct the ABI in lockstep with toolchain development and release. druntime is the only place for that. Point is, stdc++ is not a 'lib', it is effectively core language. |
|
A properly implemented systems programming language should not depend on C compilers, C++ compilers, or any other language's toolchain; it should only need the linker. The fact that D has a dependency on the C toolchain (at least for now), is a unfortunate drag; please, let's not make it worse. C++ ABI testing is the responsibility of our CIs. Our CI infrastructure can incorporate a C++ toolchain to verify D's implementation. The CIs could even import the stdc++ bindings from another repository for testing, just as we do when testing Dub packages with each change to DMD. Furthermore, we don't have the resources for this. @TurkeyMan, if this is important to you, then we need someone like you to take ownership of it, and not just leave it for the rest of us to deal with. We're already way understaffed, and the issues and PRs just keep coming. If it's yours you won't need to wait on or negotiate with anyone to get things done; and you'll be much more productive. Once you have a repository created, we can incorporate it into our CIs like we do the other Dub repositories to ensure DMD remains compatible. |
src/core/stdcpp/array.d
Outdated
|
|
||
| // accepted solution to the C++ namespace problem is to alias symbols outside of the namespace | ||
| // https://forum.dlang.org/thread/[email protected] | ||
| alias array = std.array; |
There was a problem hiding this comment.
Missing Ddoc comment.
|
I really can't see it that way... I would call the opposite; a properly implemented systems language absolutely does have intimate access to the c runtime, it's like the core native lib for any system. What resources does this require? I'm already carrying this. I expect these are very low-maintenance once they work. This is system code that almost never changes, and in the rare event they do, that's the whole point. |
|
@TurkeyMan, I'm not trying to discourage you or anyone else from doing this work; I would love to see this work done, I just don't believe it belongs in the druntime repository. Also I think progress on these bindings would be MUCH more accelerated if they were not here. In fact, putting them here will likely be a hindrance (as you are currently experiencing, and have experienced with your other PRs). Alternatives include, but are not limited to:
Please note that even if these bindings are in a different repository, they can still be included in the D compiler distribution, if someone believes that would be appropriate for some reason.
Working on team DMD and druntime is definitely NOT low-maintenance.
The maintenance for this work requires creating the bindings (yes, the majority of the work does happen up front, but it doesn't stop there), updating them as needed, responding to questions from users (on the forum, in bugzilla issues, and in PRs), debugging issues, devising solutions to any issues, implementing those solutions as PRs, reviewing those PRs for coding conventions, tests, coverage, documentation, and correct implementation, and negotiating with contributors. All that while trying to keep up with the backlog of issues and PRs we already have competing for our attention. Furthermore, these bindings require expertise and experience with C++ and stdc++. While most of us probably have some experience there, such skills are not an inherent prerequisite for maintaining druntime. To assume ownership of this, we require active team members that are very familiar with C++ and stdc++. I can't even count on one hand enough team members that would qualify as active let alone having stdc++ experience. Perhaps we could add you to the team as code owner, @TurkeyMan. Are you willing to assume that responsibility, answer questions, handle issues, review PRs, negotiate solutions with contributors, etc.? Even if we did have you, or a team with the right expertise and resources to create and maintain these bindings, I still don't believe the bindings belong here. Simply put, druntime has a different charter. There are many things in druntime that don't actually belong here. We should be trying to get rid of those things, not add to them. And where does it stop? We've recently had PRs for adding bindings and
With all that said, note that I am just one person on this team expressing my opinion and exercising whatever influence I might have. I do not speak for the whole team; each will have to voice their own opinions. I don't expect anyone to agree with me, but that doesn't mean I shouldn't articulate my point of view. I've already had my say, and maybe even said to much. I defer to those more senior than I for a decision. "Perfection is attained not when there is nothing left to add, but when there is nothing left to take away." -- Antoine de Saint-Exupery |
|
If we are going to add bindings for the C++ standard library, does that mean we're going to add bindings for the Objective-C frameworks? Where do we draw the line. |
|
I have nothing to add that @JinShil haven't said yet. It's an awesome initiative and I'm looking forward to have it available. I think it should live somewhere special (dlang-community?) and be a @TurkeyMan : Your main concern seems to be about codegen regressions. It is my understanding Buildkite would solve it as efficiently as putting the code in druntime would. Am I missing something ? |
|
I don't, and don't intend to ever use dub :/ If it's added to the CI, then what's the practical difference from placing it here? It will have the same effect in terms of maintenance. I still can't really understand the argument. It's about maintenance, but initial development of 3rd party interaction is a lot of additional scripting, and ongoing maintenance are all only harder if it's in some 3rd party location...
If you accept this, then I don't understand how integrating another library parallel to druntime with the same build, CI, and distribution semantics as druntime isn't more work... it just... is more work, for the same outcome no? Anyway. I yield. |
|
Test is no longer run on OSX32, if clean is run |
|
Generally speaking it shouldn't be that difficult to maintain old ports. Solaris 10 and OSX 10.6 will be baselines of gcc 9 for those respective platforms. There should be better way to manage transitions though. For instance, only supporting one version of FreeBSD is a pain, not having a clear |
|
I agree about the |
|
@TurkeyMan please squash this after I'm done with it. |
@andralex Thanks man! |
|
Considering we are. No funds yet. |
|
I'm currently working as a DevOps, professionally. |
|
Woah! That was sudden... I would have liked to squash Nick's stream of consciousness (or even reviewed it) ;) |
|
But thanks! That was a grueling marathon! |
|
Fwiw Buildkite is intended to replace the auto-tester, but we currently don't have Windows nor OSX agents connected to it :/ |
|
Buildkite... that doesn't even let you see the state of the builds! |
|
See slack. |
What about the existing machines used for the auto-tester? |
|
We don't have access to them. |
If we know how owns them we could ask. |
|
See e.g. braddr/d-tester#64 |
|
I don't follow. We don't want to continue using those machines because they're old or physical? |
|
No because we can't install the buildkite agent on them because we don't have access. That was just a link to a previous attempt at improving the hardware setup. |
First and simplest of the bunch. I'd like to get this in to establish layout and styling for the others.