Fix issue 13348 - byGrapheme is not pure #5723
Conversation
|
Thanks for your pull request, @DmitryOlshansky! Bugzilla references
|
std/uni.d
Outdated
| import std.traits; // isConvertibleToString, isIntegral, isSomeChar, | ||
| // isSomeString, Unqual | ||
| import std.exception : enforce, collectException; | ||
| import core.memory : pureMalloc, pureRealloc, pureFree; |
There was a problem hiding this comment.
Can you use function local imports instead? There are still some issues with top level selective imports: https://issues.dlang.org/show_bug.cgi?id=17630
There was a problem hiding this comment.
Honestly that was an attempt to solve linker error while using function local imports...
std/uni.d
Outdated
| // kill unrolled switches | ||
|
|
||
| private static bool isRegionalIndicator(dchar ch) @safe | ||
| private static bool isRegionalIndicator(dchar ch) @safe pure @nogc |
DmitryOlshansky
force-pushed
the
fix-issue-13348
branch
from
7bc5d84 to
7c47c11
Compare
|
@JackStouffer any ideas on how do I restart circle-ci test ? |
|
This is one way |
|
huh, usually that works cc @wilzbach |
|
You can restart CircleCI by logging with your GitHub profile there. After that a Restart Build button should show. (I've just done so.) |
PetarKirov
left a comment
PetarKirov
left a comment
There was a problem hiding this comment.
LGTM, modulo some minor comments.
std/uni.d
Outdated
| See_Also: $(LREF Grapheme.valid) | ||
| +/ | ||
| ref opOpAssign(string op)(dchar ch) | ||
| ref opOpAssign(string op)(dchar ch) pure @nogc |
There was a problem hiding this comment.
Are pure and @nogc inferred if you remove them from the signature? If yes, can we remove them (I guess that they were useful to you when debugging the attributes on other code), if no, can you also add nothrow?
std/uni.d
Outdated
| entirely. | ||
| +/ | ||
| @property bool valid()() /*const*/ | ||
| @property bool valid()() pure @nogc/*const*/ |
There was a problem hiding this comment.
Dittto - is keeping the attributes really necessary?
std/uni.d
Outdated
| } | ||
|
|
||
| this(this) | ||
| this(this) pure @nogc |
std/uni.d
Outdated
| } | ||
|
|
||
| ~this() | ||
| ~this() pure @nogc |
std/uni.d
Outdated
| } | ||
|
|
||
| void convertToBig() | ||
| void convertToBig() pure @nogc |
std/uni.d
Outdated
|
|
||
| import std.exception : enforce, collectException; | ||
| import core.memory : pureMalloc, pureRealloc, pureFree; | ||
| import core.exception : onOutOfMemoryError; |
There was a problem hiding this comment.
What error do you get if you make those imports function local? Also if you can't make them function local, you should make them non-selective as currently there's a compiler bug causing them to behave as they were public.
There was a problem hiding this comment.
Link errors pointing roughly in the direction of druntime. Will make non-selective.
There was a problem hiding this comment.
Can you file a bug for this? I guess it would too hard to minimize by hand, so something along the lines of "At phobos commit xxxx, if the imports at file:line are made selective, I get this link error..." would be good enough.
There was a problem hiding this comment.
DmitryOlshansky
force-pushed
the
fix-issue-13348
branch
from
7c47c11 to
187e2b7
Compare
| import std.exception;// : enforce; | ||
| import core.memory; //: pureMalloc, pureRealloc, pureFree; | ||
| import core.exception; // : onOutOfMemoryError; | ||
| static import std.ascii; |
There was a problem hiding this comment.
What error do you get if you make those imports function local? Also if you can't make them function local, you should make them non-selective as currently there's a compiler bug causing them to behave as they were public.
@ZombineDev It's the other way around: every new non-selective import we add, is public.
In other words, the following is now possible:
import std.uni : assumeUnique, pureMalloc; // ...There was a problem hiding this comment.
Sorry I remembered that bug report backwards.
5 participants
No description provided.