Sanitize Appender constraints a bit. #2178
Conversation
| static assert(!is(Appender!(char[5]))); | ||
| static assert(!is(Appender!D)); | ||
| static assert(!is(Appender!S)); | ||
| static assert(!is(Appender!L)); |
There was a problem hiding this comment.
Shouldn't these say !is(typeof(... ? I've never seen this type of construct before.
Edit: Nevermind, I get it now. Duh...
| { | ||
| static assert (!isStaticArray!A || __traits(isRef, array), | ||
| "Cannot create Appender from an rvalue static array"); |
There was a problem hiding this comment.
I'm wondering if this is a correct approach. I think the less code that allows implicit static-array to dynamic array conversion, the better. In particular, currently, this function (mostly) never worked taking static arrays anyways. My new code adds support for them, and I'm not sure that's the right direction to go in?
Maybe it would be better to simply reject them?
Appender!(E[]) appender(A : E[], E)(auto ref A array)
if (!is(A : E[N], size_t N))
{
...
}I can't say there'd be no breakage, but it should be minimal... And it's the only way I see to make appender certifiably correct...
Thoughts?
There was a problem hiding this comment.
Static arrays should be usable by appender. Very frequently, one uses a static buffer, which will work as long as the buffer can hold the data, and then will start appending onto the heap when more data is needed. This provides a very good level of optimization where you may not know exactly how much space is needed, but you have a general idea of the common case.
In any case, supporting slices automatically supports passing in static arrays, since you can just slice it (at least today). I'm unsure of the correct answer. I think for now, just make the changes so the code is correct, and we can talk about breaking code in a different PR.
There was a problem hiding this comment.
I mostly meant having the call site take care of the slicing, rather than appender itself. In any case, after thinking about it, I think the new code is OK.
|
I think this LGTM |
|
Sight, these fixes seem like an exercise in futility: |
|
LGTM, the autotester is passing. Should we just pull this? |
|
I think it can be pulled, I typically do not want to pull something that only I have reviewed. But now you did too, so... |
|
Auto-merge toggled on |
Sanitize Appender constraints a bit.
This rewrites the template definition of
Appenderto use theisDynamicArrayconstraint: This avoids ridiculous declarations ofAppender!Awhere A is convertible to a dynamic array, but not an actual dynamic array (thinkalias thisand static array). AFAIK, this should break nothing, since the constraints should usually be explicit and correct.I did not change
appender's restraint itself, since it worked anyways, as it explicitly created anAppender!(E[])anyways, and didn't want to break working code for no reason.The bug I initially wanted to fix was related was (yet again) a "static array" => "dynamic array" implicit conversion error. Basically, static arrays were passed by value to
appender, and thenappendercreated an actualAppenderby slicing that local static array. This left the appender referencing locally scoped and later destroyed data...@schveiguy : You've been diligent at reviewing my appender fixes, give this a whirl?