Templatize _d_arrayappendcTX#21205
Conversation
|
Thanks for your pull request and interest in making D better, @Albert24GG! 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 + dmd#21205" |
teodutu
left a comment
teodutu
left a comment
There was a problem hiding this comment.
Looking good. Now fix the few coding style nitpicks and try to remove the qualifiers in the compiler instead of the wrapper hook.
|
Why were some jobs cancelled? |
schveiguy
left a comment
schveiguy
left a comment
There was a problem hiding this comment.
I dislike having to cast all over the place. Maybe better to refactor, and use T* for ptr instead of void*?
The only place where void* makes the most sense is the memset call, but you can change newsize to newlength there, and then it's fine.
In general, it would be nice to get rid of void* and variables that aren't based on the element size completely. This is one clunky piece of the original code that is always confusing (is it length or size? Which one is in terms of bytes and which is in terms of elements?). It also can limit where we have to write trusted code.
This can be done later, if you don't want to do it in this PR. But fix the other things.
The runner they're using is deprecated: actions/runner-images#11101 |
|
@schveiguy @teodutu is this PR ready to be merged? Your comments have been addressed. |
|
Please squash commits. Also, is the changelog necessary? I haven't seen this being done for other hooks. |
|
The requirement for a changelog entry was suggested here [1]. I made some changelog entries myself back in the day, but I also forgot about them. [1] #21151 (comment) |
Use existing `Unqual` and `__arrayAlloc` templates Remove redundant array assignment Update templatized `_d_arrayappendcTX` Remove old `_d_arrayappendcTX` hook Remove redundant variable Replace `size_t` with `auto` Fix slice passed to `gc_expandArrayUsed` Avoid casting pointer retuned by `GC.malloc` Remove `pure` attribute from `_d_arrayappendT` Fix spacing for `version` statements Add newline at EOF in `appending.d` Remove explicit pure attribute from `_d_arrayappendcTX` Remove implementations details from docs Trim trailing whitespace Fix spacing in if statements Remove unnecessary casts refactor: Replace `void*` with `T*` Mark gc extern(C) functions as private Add docs explaining the purpose of `@trusted` attribute Fix newlines Call `typeid` directly without storing the result Add changelog file
Albert24GG
force-pushed
the
_d_arrayappendcTX
branch
from
7c4a6f7 to
696ee49
Compare
|
@RazvanN7 Done with squashing. |
7 participants
This PR continues the work started in #21144.
Any suggestions for further cleanup or optimization are welcome.