Fix Issue 17961 - std.uni does not compile with -unittest -dip1000 #6041
Conversation
|
Thanks for your pull request and interest in making D better, @carblue! 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 references
|
std/uni.d
Outdated
| return this = This.init.add(ch, ch+1); | ||
| this = This.init; | ||
| { | ||
| data.length(0); |
There was a problem hiding this comment.
Nitpick: I think the intended usage is:
data.length = 0;
There was a problem hiding this comment.
I didn't go through all details of CowArray and Your comment was my first idea too, but as it is reference counted, I opted for the method
@Property void length(size_t len)
which cares for refCount
There was a problem hiding this comment.
@quickfur 's suggestion still calls that method:
void length (int val)
{
import std.stdio;
writeln(val);
}
void main()
{
length = 1;
}
(prints 1).
There was a problem hiding this comment.
I think just using return scope should be the ticket.
No idea what this muckering with scope here acomplishes but I think it just fools compiler
There was a problem hiding this comment.
@carblue I think you misunderstood. The syntax data.length = 0; is lowered to data.length(0). It's not a big deal, but usually when you write @property void length(size_t) { ... }, the intention is to use it as a property setter method, i.e., you would invoke it with assignment syntax rather than function call syntax.
There was a problem hiding this comment.
Agreed about data.length = 0; and thanks, I forgot about that lowering.
@DmitryOlshansky The scope topic is tricky; I watched Walter Bright's Pointers Gone Wild: Memory Safety and D several times, introduced that into some code and know from tests, that the implementation still needs some refinement: I wouldn't call it "fool", but just "help" the compiler with it's current, hypersensitive dip1000 code and/or help me save time: Whether legitimately or not (I didn't go to the bottom of that in static struct Intervals(Range) or wherever, which may be worthwhile), the compiler - in the first place - rejects byInterval in foreach with the error mentioned in issue 17961. The simple, yet maybe temporary solution I opted for, is, to give the compiler what it seemingly wants to see here, a scope variable (arr) -based iteration.
I hope, when dmd is dip1000-ready, the compiler will be that 'clever' to detect the helping hand case here, making arr dispensable.
If I understood Your ticket right, 'return scope this;' doesn't compile for syntax reason.
std/uni.d
Outdated
| data.length(0); | ||
| return this.add(ch, ch+1); | ||
| } | ||
| data.length(0); |
quickfur
left a comment
quickfur
left a comment
There was a problem hiding this comment.
Otherwise, LGTM.
But I'll wait for @DmitryOlshansky to take a look first.
std/uni.d
Outdated
| return this = This.init.add(ch, ch+1); | ||
| this = This.init; | ||
| { | ||
| data.length(0); |
There was a problem hiding this comment.
I think just using return scope should be the ticket.
No idea what this muckering with scope here acomplishes but I think it just fools compiler
|
LGTM. Though I'll defer to @DmitryOlshansky to take another look at the new proposed fix. |
|
Errors on circle-ci and ddoc-tester seem spurious, could you please rebase? |
|
Yes I will rebase, but first learn how to do that. |
|
CircleCi runs The target ensures that the public unittest in Phobos don't contain any private dependency and are really runnable. It does nothing more than extracting all public unittest and runs them against the freshly compiled Phobos library. |
|
BTW here's a simple trick that allows to gradually enable diff --git a/posix.mak b/posix.mak
index d0576574a..e30b687c7 100644
--- a/posix.mak
+++ b/posix.mak
@@ -357,7 +357,10 @@ UT_D_OBJS:=$(addprefix $(ROOT)/unittest/,$(addsuffix .o,$(D_MODULES)))
$(UT_D_OBJS): $(ALL_D_FILES)
$(UT_D_OBJS): $(ROOT)/unittest/%.o: %.d
@mkdir -p $(dir $@)
- $(DMD) $(DFLAGS) $(UDFLAGS) -c -of$@ $<
+ declare -A aa=( \
+ ["std/uni.d"]=-dip1000 \
+ ) \
+ $(DMD) $(DFLAGS) $(UDFLAGS) $${aa["$<"]} -c -of$@ $<
ifneq (1,$(SHARED)) |
|
@wilzbach Thank You very much for Your last 2 comments. They are precisely what I currently required and enable me to go on pushing forward -dip1000 for phobos. Btw, I did the rebase asked for and it's exactly summarizing the 3 outdated commits, which did pass jenkins and IIRC auto-tester as well (now they seem to have stalled). Since 2.078.1 is released, the rebase to stable seems superfluous now, but I'll pay attention next time. |
You are welcome. Let me know if you have further questions / problems. I also sent you an invite to our Slack channel - it's often the quickest way to get answers.
Auto-tester will invalidate all results after every PR merged to dmd,druntime and Phobos. Don't worry about it being pending, that's the case for most PR. Once you new commits are added to a PR or you force-push, you will land on top of the queue.
This is just a general warning which we display for issues labelled as regression, but technically this isn't a regression, because
We still need to find out why unittest
{
import std.datetime.interval;
import std.datetime.date : Date, DayOfWeek;
auto interval = Interval!Date(Date(2010, 9, 2), Date(2010, 9, 27));
} |
|
Thanks for Your detailed explanations and the invitation to join the dlang team on slack. I registered and will explore slack tomorrow. |
|
@quickfur FYI the CircleCi failure here isn't random or spurious, it's a real one. See the discussions above. |
|
Ah, I missed that. Sorry about that. |
|
~/gitclones/phobos$ make -j6 -f posix.mak unittest @wilzbach |
|
Great job with creating the list! We are currently having some troubles with this CircleCi check, so it might be that the failure here was just the first indication of a wider problem. I will have a more detailed look into this later. |
Managed to find the cause of the permanent CircleCi issues: #6099, but I think your failure is different (see my comment above) |
Please don't use merge, you can use rebase instead. This helps to keep the history sane. See also: https://wiki.dlang.org/Starting_as_a_Contributor#Rebasing |
|
The solution will be far more complicated than/different from simply adding scope to byInterval() (which seems to work for InversionList!GcPolicy only). The impure errors stem from InversionList!ReallocPolicy. |
|
@wilzbach @DmitryOlshansky @quickfur |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Looks good, but be a bit careful with @trusted.
std/uni.d
Outdated
| import std.format : format; | ||
| enum maxBinary = 3; | ||
| static string linearScope(R)(R ivals, string indent) | ||
| @safe static string linearScope(R)(R ivals, string indent) |
std/uni.d
Outdated
| } | ||
|
|
||
| static void destroy(T)(ref T[] arr) | ||
| @trusted static void destroy(T)(scope ref T[] arr) |
There was a problem hiding this comment.
Freeing memory is not safe in general. Move the @trusted to the call site where track of the reference is kept.
There was a problem hiding this comment.
Yes, I agree about alloc, realloc and destroy, but in the end: I didn't change their attribute @trusted, they already were declared that way by original @trusted struct GcPolicy and @trusted struct ReallocPolicy. As I outlined in #6104, I currently won't touch function bodies (as far as possible) and be focussed on function/struct signatures for -dip1000 compliance. Frankly speaking, looking deeper into phobos, I was surprised how much @trusted still is in there, part of which possibly undermining memory safety, thus I will leave Your issue for Dmitry or whoever.
Reviewing my changes, there is only 1 place, where I added @trusted where no one was before: mixin template ForwardStrings function fwdStr. I forgot to mention that.
std/uni.d
Outdated
| } | ||
|
|
||
| static T[] realloc(T)(T[] arr, size_t size) | ||
| @trusted static T[] realloc(T)(scope T[] arr, size_t size) |
There was a problem hiding this comment.
destroy isn't safe in general, move this to the call site.
Ehm I would even go one step further and remove the
Yes those are unrelated. I will look into those. A start for the Jenkins failures: dlang/ci#163 |
|
@wilzbach -added unittesting of InversionList.this(Range)(Range intervals) to calm codecov (required anyway but should ideally go in isolated PR) |
Yes, restarted them.
Nice! BTW since #6124 CodeCov will always be green / "calm". |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
checked for all removed with pragma(msg typeof...)
I think it would be better to check that the auto-inferred tests have at least one @safe unittest, s.t. they don't accidentally get broken in the future.
The same for removing pure and letting the compiler do inference.
Otherwise the changes look even better than before. Great job!
| mixin template ForwardStrings() | ||
| { | ||
| private bool fwdStr(string fn, C)(ref C[] str) const pure | ||
| private bool fwdStr(string fn, C)(ref C[] str) const @trusted |
There was a problem hiding this comment.
Do we have a test for the purity here?
There was a problem hiding this comment.
Yes, with latest commit: Any utf\d*Matcher calling match/test/skip has fwdStr in it's call chain, e.g. pure @safe unittest beginning at line 4634 or 4670 or 5319 or 5374 or 5424. Added pure to many @safe unittest.
BTW, I failed introducing Mallocator in ReallocPolicy.alloc/realloc/destroy due to @System calls, like this:
@@ -1812,6 +1812,7 @@
@safe struct ReallocPolicy
{
import std.range.primitives : hasLength;
+ import std.experimental.allocator.mallocator : Mallocator;
static T[] dup(T)(const T[] arr)
{
@@ -1820,9 +1821,10 @@
return result;
}
- static T[] alloc(T)(size_t size) @trusted
+ static T[] alloc(T)(size_t size) //@trusted
{
- import core.stdc.stdlib : malloc;
+// import core.memory : pureMalloc; //import core.stdc.stdlib : malloc;
+ import std.experimental.allocator : makeArray;
import std.exception : enforce;
import core.checkedint : mulu;
@@ -1830,13 +1832,14 @@
size_t nbytes = mulu(size, T.sizeof, overflow);
if (overflow) assert(0);
- auto ptr = cast(T*) enforce(malloc(nbytes), "out of memory on C heap");
- return ptr[0 .. size];
+ return enforce(Mallocator.instance.makeArray!ubyte(nbytes), "out of memory on C heap"); //auto ptr = cast(T*) enforce(pureMalloc(nbytes), "out of memory on C heap");
+// return ptr[0 .. size];
}
Out of interest: what do you mean by this? |
|
After commit 2f590da I checked the cyberShadow/DAutoTest generated |
|
Ah don't worry about this, but the backend (run.dlang.io) can't build Docker containers for every PR. We simply use master (aka dmd-nightly) for the preview documentation. If you want to learn more: https://dlang.org/blog/2017/03/08/editable-and-runnable-doc-examples-on-dlang-org |
|
I'm aware, that there's still a lot to be done for std/uni.d, e.g. looking at it's unittest-coverage, but I try to not get bogged down, focus on -dip1000 first and DIP1000.md is 'fresh' in my brain: Thus the reverted unittest will come back later again at another (non-public unittest) location. |
|
This PR expired my "languishing acceptance", just closing it now |
|
Please don't close PRs based on arbitrary time limits. I simply do not have time to work on it at the moment. That doesn't make it a bad PR. |
|
This is on my TODO-List for a new PR, as proposed by @wilzbach : |
[Trivial]std.uni: Fix a -dip1000 compilable issue; appeared since #6041
7 participants
see also my comment #7 in issue 17961