Trim as much version(unittest) as possible from allocator #6452
Conversation
schveiguy
requested review from
PetarKirov,
andralex and
wilzbach
as code owners
|
Thanks for your pull request, @schveiguy! 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 + phobos#6452" |
| void* m; | ||
|
|
||
| @nogc nothrow | ||
| size_t addr() { return cast(size_t) m; } |
There was a problem hiding this comment.
I moved this in the unittest, even though the version block had no imports, as I can't imagine there's a good reason for adding addr to the global namespace as a public symbol. The loss of UFCS is mitigated by the fact that we always were calling this on m, so it actually gets shorter.
There was a problem hiding this comment.
The loss of UFCS might make this less intuitive. I know I had to double check what addr does. What is the gain in changing the function signature?
Do the function attributes and the function definition have different indents or is it just the web display?
There was a problem hiding this comment.
If I didn't change the function (and just made it a static), then it would become addr(m). I changed it to m_addr to make it more obvious what it is.
The attributes were indented differently. This is what my editor did. But I have redone the function into a lambda, which looks quite nice.
std/experimental/allocator/common.d
Outdated
| // unnecessary import of std.experimental.allocator | ||
| import std.experimental.allocator : RCIAllocator, RCISharedAllocator; | ||
| static assert(is(RCAllocInterface == RCIAllocator) | ||
| || is (RCAllocInterface == shared RCISharedAllocator)); |
There was a problem hiding this comment.
While you are at it, you could also remove the shared keyword as this was before declaring RCISharedAllocator as a shared struct.
| void* m; | ||
|
|
||
| @nogc nothrow | ||
| size_t addr() { return cast(size_t) m; } |
There was a problem hiding this comment.
The loss of UFCS might make this less intuitive. I know I had to double check what addr does. What is the gain in changing the function signature?
Do the function attributes and the function definition have different indents or is it just the web display?
|
Fixed nits, let me know what you think on the |
|
I think this solves the (my 😄 ) "problem" perfectly |
|
Don't you have to call |
|
Hah! the tests passed my mac, forgetting that this was a windows-specific test. |
|
I thought there was a way to alias a function lambda so that it becomes a real function? Oh well, will revert back to a function, but we don't need the attributes, they should be inferred. |
|
@edi33416 fixed. Not as pretty, but still short. Hopefully it passes this time, I tested something similar on my mac. |
3 participants
Most of the
version(unittest)stuff here is not needed at all (there's no reason to guard unittests withversion(unittest)).A couple of the changes are not trivial, however.
See #6450 for more details.