Add @nogc variants for array utils functions#3583
Conversation
Also use enforceRawArraysConformableNogc in _d_arrayctor Signed-off-by: Teodor Dutu <teodor.dutu@gmail.com>
|
Thanks for your pull request and interest in making D better, @teodutu! 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 run digger -- build "master + druntime#3583" |
|
|
||
| alias Type = void function(const char[] action, const size_t elementSize, | ||
| const void[] a1, const void[] a2, in bool allowOverlap = false) @nogc pure nothrow; | ||
| (cast(Type)&enforceRawArraysConformableNogc)(action, elementSize, a1, a2, false); |
There was a problem hiding this comment.
This is a void nothrow pure function. Are we sure that the compiler does not optimize away calls to it?
There was a problem hiding this comment.
I've compiled and disassembled the code with optimisations turned on (-release -O -inline -boundscheck=off) and off and enforceRawArraysConformable was performing the function call. However, I've only performed this check on my Linux x64 machine.
Is there a way I can enforce the call to enforceRawArraysConformableNogc regardless of optimisations?
| const overlappedBytes = bytes - d; | ||
|
|
||
| assert(0, errorMessage("Overlapping arrays in %s: %zu byte(s) overlap of %zu", | ||
| action, overlappedBytes, bytes)); |
There was a problem hiding this comment.
Wait, how is this @nogc? It's a throw new AssertError(…) when compiling druntime with enabled assertions. If it really was @nogc, then there'd be no need to hack-casting the function type.
There was a problem hiding this comment.
Oh, it defaults to throwing a staticError!AssertError, so assert() is @nogc (although the custom assert handler might use it AFAICT). Anyway, the functions can apparently be properly decorated with @nogc.
As DMD ships with release druntime only, the assert(0) boils down to a rather unhelpful invalid-instruction, no msg is displayed. It might make sense to invoke _d_assert_msg manually to make sure the message is actually used.
There was a problem hiding this comment.
Oh, it defaults to throwing a staticError!AssertError, so assert() is @nogc (although the custom assert handler might use it AFAICT) Anyway, the functions can apparently be properly decorated with @nogc.
Yes.
there'd be no need to hack-casting the function type
You're right. I added the cast there more for the purpose of clarity than out of an actual need. I should probably remove it.
Anyway, the functions can apparently be properly decorated with @nogc.
They can. I chose not to because @nogc would have been inferred anyway, but I see your point.
As DMD ships with release druntime only, the assert(0) boils down to a rather unhelpful invalid-instruction, no msg is displayed. It might make sense to invoke _d_assert_msg manually to make sure the message is actually used.
I think you're right here. I'm going to change the asserts into _d_assert_msgs
There was a problem hiding this comment.
Thanks!
I chose not to because @nogc would have been inferred anyway, but I see your point.
Attributes are only inferred for templates and auto return types.
| { | ||
| import core.stdc.stdio : snprintf; | ||
| snprintf(&_store[0], _store.sizeof, format, &action[0], args); | ||
| return _store; |
There was a problem hiding this comment.
This returns the full buffer. You'll probably want to return the actual payload only, using the snprintf return value (see its spec, it might return a number greater than the buffer size if it wasn't sufficiently large).
There was a problem hiding this comment.
Yes, I can do that, too. Also, snprintf returns the number of bytes written, so in this case it cannot return more that _store.sizeof, i.e. 256. It could return a negative value, meaning an output error, but that's besides the point and nearly impossible to happen here.
There was a problem hiding this comment.
See https://www.cplusplus.com/reference/cstdio/snprintf/:
Return Value
The number of characters that would have been written if n had been sufficiently large, not counting the terminating null character.
There was a problem hiding this comment.
Also note that you are assuming action is null-terminated (because the format string uses %s and you're passing the pointer only). %.*s is the usual way of preventing this undocumented assumption.
4 participants
This PR also replaces the error messages in
_d_arrayctorwith those provided byenforceRawArraysConformableNogc.The previous error messages in
_d_arrayctorwere displaying too few details, because the initialenforceRawArraysConformablefunction could not be used, as it was not@nogc. This decision was described in this PR: #3582.