Convert _d_arraycast to template #2264

JinShil · 2018-08-02T03:14:48Z

This is all dead code (well, except for the unittest). The idea is to verify this implementation first, merge it, then merge dlang/dmd#8531 (assuming it passes all tests).

dlang-bot · 2018-08-02T03:14:49Z

Thanks for your pull request, @JinShil!

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 + druntime#2264"

Geod24 · 2018-08-02T03:56:18Z

src/object.d

+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted
+{
+    auto nbytes = from.length * TFrom.sizeof;
+    assert((nbytes % TTo.sizeof) == 0, "array cast misalignment");


This probably should be a proper error. Because if someone compiles in -release, this check is gone.
Also the error message should be more descriptive.

I can't make this an Error, or it won't work in @nogc code. See this comment for a more detailed explanation.

Because if someone compiles in -release, this check is gone.

That's a good point that I hadn't considered. Note that if the assert is removed, this code can no longer be considered @trusted.

@wilzbach said:

Catching Error is defined to result in undefined behavior. We don't need to cater for such code. So you can use assert?

In an e-mail to me, @WalterBright said:

I agree with Seb. It should have been implemented that way the first time around.

So, now that I've been directed to staticError (Thanks @Geod24) I can throw Errors in @nogc code, but given @WalterBright's response, I'm not sure what I should do.

Geod24 · 2018-08-02T03:56:33Z

src/object.d

+ */
+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted
+{
+    auto nbytes = from.length * TFrom.sizeof;


const nbytes = ...

Geod24 · 2018-08-02T03:57:20Z

src/object.d

+    return *cast(TTo[]*)a;
+}
+
+unittest


@safe @nogc pure nothrow

Geod24 · 2018-08-02T03:58:08Z

src/object.d

+    assert(s.length == 6);
+
+    s = __ArrayCast!(int, short)(i);
+    assert(s.length == 6);


I'd also do an array comparison, or at the very least, a first-element comparison in addition to this length check

Geod24 · 2018-08-02T06:29:50Z

src/object.d

+{
+    const auto nbytes = from.length * TFrom.sizeof;
+
+    if ((nbytes % TTo.sizeof) != 0)


I would additionally nest this in:

version (D_NoBoundsChecks) {} // Oh, the irony else { if ((nbytes...)) ... }

but that might be up for discussion

Geod24 · 2018-08-02T06:30:10Z

src/object.d

+ */
+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted
+{
+    const auto nbytes = from.length * TFrom.sizeof;


A storage class is enough, so auto is redundant

Geod24 · 2018-08-02T06:30:53Z

src/object.d

+
+    i = __ArrayCast!(byte, int)(b);
+    assert(i.length == 3);
+    foreach(v; i)


Here and later instances are missing a space: foreach (v; i)

Geod24 · 2018-08-02T06:41:01Z

src/object.d

+        version(D_Exceptions)
+        {
+            import core.exception : onArrayMisalignmentError;
+            onArrayMisalignmentError();


I think we should pass more informations to this function.

I'd like to see a message similar to this:

Error: Cannot cast `int[5]` to `ulong[]`, as size `20` is not aligned on `8` bytes.

It does not need to allocate. If you pass the elements separately, you can then provide a toString overload which accepts a sink.

Let's not repeat the mistake that is array bounds mismatch's error message.

Is your suggestion to have the Error's msg say "Array cast misalignment", and then have the overloaded toString function return the more elaborate error message?

I forgot people might be using msg directly... Although for Error it's unlikely. I think your suggestion makes sense.

Geod24 · 2018-08-02T08:08:18Z

src/object.d

+
+    if ((nbytes % TTo.sizeof) != 0)
+    {
+        version(D_Exceptions)


Also missing space: version (

Geod24 · 2018-08-02T08:08:50Z

src/core/exception.d

+ */
+class ArrayMisalignmentError : Error
+{
+    @safe pure nothrow @nogc this(string file = __FILE__, size_t line = __LINE__, Throwable next = null )


attributes should go on the RHS. Also, trailing space between null and closing )

Geod24 · 2018-08-02T08:09:07Z

src/core/exception.d

+{
+    @safe pure nothrow @nogc this(string file = __FILE__, size_t line = __LINE__, Throwable next = null )
+    {
+        super( "Array misalignment", file, line, next );


Extra space: super(args);

JinShil · 2018-08-02T10:41:21Z

src/core/exception.d

+        import core.internal.string : unsignedToTempString;
+        import core.stdc.stdlib : alloca;
+
+        const(char)[][8] msgComponents =


How about this for formulating a more detailed error message. I'm trying to avoid using the C standard library (alloca actually isn't part of the C standard library).

I'm trying to avoid using the C standard library

Why ?
Also, I would suggest to just store those values as field and do the formatting in toString

Why ?

Because eventually I'd like to jettison it completely from druntime. I also need to consider using the same code for the assert message in __ArrayCast.

I need to come up with a utility for creating a dynamic string on the stack.

I ended up merging the message generation code into the __ArrayCast implementation itself so I could use it for the assert as well.

JinShil · 2018-08-02T13:25:33Z

src/core/exception.d

+ * Throws:
+ *  $(LREF Error).
+ */
+void throwStaticError(string msg, string file = __FILE__, size_t line = __LINE__) @trusted pure nothrow @nogc


This just throws a generic Error. I don't know what the policy is on strongly typed exceptions.

What's the reason for this function?

See the body of the function: throw staticError!Error(msg, file, line, null);. That template instantiates a static Error so that it can be used in @nogc code. However, it is private. I created the throwStaticError function to keep the staticError template private and still allow throwing static Errors.

Edit: The patterns currently employed in core.exception for throwing static Errors don't make much sense to me. The Errors are all strongly typed and are encapsulated behind a extern(C) void on{StronglyTypedError}() function. I don't know why this was done; I certainly would have done it differently. It seemed so simple to just create this throwStaticError function rather than create a new strongly typed error and another extern(C) void on{StronglyTypedError}() wrapper.

To be able to throw an error in @nogc.

I created the throwStaticError function to keep the staticError template private and still allow throwing static

What's the advantage of this wrapper function compared to using staticError directly? I don't see how introducing a new public symbol to keep an existing symbol private is an advantage. You can make the existing staticError package to keep it internal to druntime.

I can't make it package because that would only make it visible to modules within core.

Hmm, right. This is in the object module which is not inside any package. But again, is this any better than making the existing staticError public?

No, it's not any better IMO. If we decide to use static Errors, I think I'll just follow the existing pattern for other static Errors in core.exception. That is, create a strongly typed Error and throw it with an on{whatever} function.

jacob-carlborg · 2018-08-02T14:15:08Z

src/object.d

+        }
+        else
+        {
+            assert(false, cast(string)msg);


I don't know how assert(false) behaves but assert(0) will halt the program when assertions are turned off.

assert(false will do the same because it's the same: https://run.dlang.io/is/AVSUFK

@JinShil we typically write assert(0, ...

we typically write assert(0, ...

IMO we should stop doing that. It's a condition, not a number.

@jacob-carlborg I tested this out and you're right. If the first argument is 0 or false it still runs when asserts are disabled. However, I can find no mention of that behavior in the spec. If this is by design, it'd great if you could document it, and then I could potentially remove throwing the static Error altogether.

Kindly asking for advise from all reviewers on how to proceed.

Found this in the spec.

The implementation may handle the case of the first AssignExpression evaluating at compile time to false differently in that in release mode it may simply generate a HLT instruction or equivalent.

I guess I'l just remove the static Error and rely on assert(false) for all implementations, then. It should keep the functions @trusted even in release mode.

I guess I'l just remove the static Error and rely on assert(false) for all implementations

Please don't. At the very least the behavior should be the same as array bounds check.
Additionally, outright crashing the program is not something that other parts of the runtime do (and there's many arguments for that). The common usage it to throw an Error.

Please don't.

I'd be happy to revert back to throwing a static Error and assert only for betterC-like builds, but I need consensus from reviewers or decree from @WalterBright

I know there's been some disagreement about the behavior of assert(0/false) because not all CPUs have the HLT instruction, but perhaps that covered with the "or equivalent" in the spec.

wilzbach · 2018-08-02T15:28:02Z

src/object.d

+            import core.exception : throwStaticError;
+            throwStaticError(cast(string)msg);
+        }
+        else


It looks like D is moving in C++ land here:

Divergent error handling has fractured the C++ community into incompatible dialects:
(1) C++ projects often ban even turning on compiler support for exception handling, but this means they are not using Standard C++. Exceptions are required to use central C++ standard language features (e.g., constructors) and the C++ standard library. Yet in [SC++F 2018], over half of C++ developers report that exceptions are banned in part (32%) or all (20%) of their code, which means they are using a divergent language dialect with different idioms (e.g., two-phase construction) and either a nonconforming standard library dialect or none at all. We must make it possible for all C++ projects to at least turn on exception handling support and use the standard language and library.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0709r0.pdf

Indeed. That's what happens when a language is not designed from the beginning with freestanding, resource-constrained systems and scalability in mind. The emergence of things like @nogc, betterC, etc. shows that D is backtracking, trying to undo some mistakes of the past. GC should be an opt-in library feature and exceptions should not be rooted in Object or Java-style class-like types. But that's what we have. If we want D to scale to systems other than resource-abundant PCs and servers running full-featured operating systems, then we have to offer alternatives. I hope future language decisions will keep principles of minimalism in mind. Given what's possible in D, I'd prefer if classes were just deprecated, and structs were give a few more features.

I'd prefer if classes were just deprecated, and structs were give a few more features.

Why? There are still people out there that still uses classes.

JinShil · 2018-08-03T07:20:01Z

src/object.d

+ * Returns:
+ *  `from` reinterpreted as `TTo[]`
+ */
+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted


I'm also wondering about code coventions here: Should it be (TFrom, TTo) or (TTo, TFrom). I took wisdom from std.algorithm.mutation.copy, but I'm not sure if any wisdom was used in deciding that convention. This will come into play in future PRs I have planned, also.

Perhaps use the same order as the cast expression.

The only rule I know of is to put arguments that needs to be explicitly specified first, otherwise follow the order or arguments. Additionally, in very rare cases, you might have 2 templates argument depending on each others (e.g. via a constraint) in which case the order is self-evident.

In this case, this is never called directly by the user so that order doesn't matter much.
Personally, I find TFrom, TTo (as currently specified) to be the clearer alternative.

Shouldn't array casting be also pure and @nogc?

Ah, the style policy on template attributes has changed. Good!
dlang/dlang.org@f95cb98#diff-2ceb1eaea6947a0a0ad321e635cec2e0

- $(LI $(I Templated) functions should $(B not) be annotated with attributes - as the compiler can infer them.) + $(LI If the template arguments for a $(I templated) function affect whether + an attribute is appropriate, then the function should $(B not) be + annotated with that attribute so that the compiler can infer it. + However, if the attribute is not affected by the template arguments + (and thus would always be inferred), then the function should be + explicitly annotated with that attribute just like a non-templated + function would be.)

wilzbach · 2018-08-03T11:33:56Z

src/object.d

+        const errorMessage = generateArrayCastErrorMessage(TFrom.stringof, fromSize, TTo.stringof, toLength * TTo.sizeof);
+
+        // must be `false` or `0` to ensure the assertion remains
+        // even when compiling with `-release`


That comment is a bit misleading to future readers as the assert(0) expression gets replaced by a HLT instruction in -release ...)

n8sh · 2018-08-03T18:49:38Z

src/object.d

+        onArrayCastError(TFrom.stringof, fromSize, TTo.stringof, toLength * TTo.sizeof);
+    }
+
+    struct Array


Does this need static?

Since there is no functions, it does not

n8sh · 2018-08-03T20:48:07Z

src/object.d

+    const fromSize = from.length * TFrom.sizeof;
+    const toLength = fromSize / TTo.sizeof;
+
+    if ((fromSize % TTo.sizeof) != 0)


Making sure that @Geod24's suggestion in #2264 (comment) isn't lost:

version (D_NoBoundsChecks) {} else if ((fromSize % TTo.sizeof) != 0)

I'm waiting to find out what happens with dlang/dmd#8532. I don't think it should hold up this PR, and would even be better if it were done in a followup PR.

wilzbach · 2018-08-07T15:58:40Z

src/object.d

+ *  toSize   = total size in bytes of the array being cast to
+ * Returns:
+ *  a descriptive error message describing the cast failure
+ */


FWIW for the documentation headers, we try the following:
...

Documentation comments should not use more than two stars /** or two pluses /++ in the header line.
...

Documentation comments should not have leading stars on each line.

https://dlang.org/dstyle.html#phobos_documentation

It looks like the rest of object.d doesn't follow this convention. Is it more important to keep a consistent style within a document?

@n8sh, you are right, and I doubt anyone would disagree with you, but @WalterBright has placed restrictions on large-scale changes to the code base, forcing us to resort to incremental changes to achieve consistent coding conventions. So, unfortunately, this is how we're going to get there.

wilzbach · 2018-08-07T16:03:38Z

src/object.d

+    i = __ArrayCast!(byte, int)(b);
+    assert(i.length == 3);
+    foreach (v; i)
+        assert(v == 0);


Maybe use something more interesting as values than zeros?

wilzbach · 2018-08-07T16:05:24Z

src/object.d

+ * Returns:
+ *  `from` reinterpreted as `TTo[]`
+ */
+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted


Shouldn't array casting be also pure and @nogc?

n8sh · 2018-08-07T16:52:51Z

src/object.d

+ * Returns:
+ *  `from` reinterpreted as `TTo[]`
+ */
+TTo[] __ArrayCast(TFrom, TTo)(TFrom[] from) @trusted


Can inout be used here to reduce template bloat?

n8sh · 2018-08-07T17:26:33Z

To speed things along I've edited the commit to incorporate minor changes requested by @wilzbach: explicit @nogc pure and not using all zeroes in the unittest.

JinShil · 2018-08-09T02:10:15Z

Sorry for the late change, but I realized I had to make unsignedToTempString a template so it would work in -betterC.

Geod24 · 2018-08-09T02:19:57Z

src/object.d

+
+    // convert discontiguous `msgComponents` to contiguous string on the stack
+    size_t length = 0;
+    foreach(m ; msgComponents)


foreach (

Geod24 · 2018-08-09T02:20:28Z

src/core/internal/string.d

-char[] unsignedToTempString(ulong value, return scope char[] buf, uint radix = 10) @safe
+/**
+Converts an unsigned integer value to a string of characters. This
+implemenation is a template so it can be used when compiling with -betterC


Regarding DDOC:

The first line is a "title", so it should usually be a one liner/sentence

I think the preferred style is to have 1 blank line between sections (e.g. before Params: / Returns:)

implemenation

Would pragma(inline, true) help in this case ?

Would pragma(inline, true) help in this case ?

Yeah, maybe. The source code would be available in -betterC builds. GDC doesn't currently implement pragma(inline, true), though I think it's coming. 🤔 Yeah, let's go with that for now. I can always change it to a template later if it doesn't work out.

EDIT: No, I can't do it. It only works when compiling with -inline.

The first line is a "title", so it should usually be a one liner/sentence

Yes everything up to the first blank line is considered the title and therefore should be short. After the blank line the description follows which can be long.

I think the preferred style is to have 1 blank line between sections (e.g. before Params: / Returns:)

I prefer that.

JinShil requested review from andralex and MartinNowak as code owners August 2, 2018 03:14

JinShil mentioned this pull request Aug 2, 2018

Lower _d_arraycast to object.__ArrayCast dlang/dmd#8531

Closed