Skip to content

std.datetime.systime - fix a -dip1000 compilable issue; trivial #6181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlang-bot merged 1 commit into from
Feb 18, 2018
Merged

std.datetime.systime - fix a -dip1000 compilable issue; trivial #6181

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions std/datetime/systime.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8981,7 +8981,7 @@ private:
/+
Returns $(D stdTime) converted to $(LREF SysTime)'s time zone.
+/
@property long adjTime() @safe const nothrow
@property long adjTime() @safe const nothrow scope
Copy link
Member

@jmdavis jmdavis Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of scope on either adjTime? They don't return by ref or accept anything by ref. I don't see any reason for scope to be involved.

Copy link
Contributor Author

@carblue carblue Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All related errors reported with -dip1000 switch:
std/datetime/systime.d(2635): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.add!"years".add
std/datetime/systime.d(2846): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.add!"years".add
std/datetime/systime.d(3188): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.add!"months".add
std/datetime/systime.d(3535): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.add!"months".add
std/datetime/systime.d(4004): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"months".roll
std/datetime/systime.d(4383): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"months".roll
std/datetime/systime.d(4758): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"days".roll
std/datetime/systime.d(5019): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"hours".roll
std/datetime/systime.d(5230): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"minutes".roll
std/datetime/systime.d(5419): Error: reference to local variable sysTime assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"seconds".roll
std/datetime/systime.d(5554): Error: reference to local variable st assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"msecs".roll
std/datetime/systime.d(5684): Error: reference to local variable st assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"usecs".roll
std/datetime/systime.d(5826): Error: reference to local variable st assigned to non-scope parameter this calling std.datetime.systime.SysTime.roll!"hnsecs".roll

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1000.md#scope-function-returns
In that section, the examples that have comment "applies to 'this' reference", thus scope on the left-hand side syntax requires e.g. a colon added as well (but doesn't work, seemingly unimplemented). The scope keyword (lifetime control) in all locations I touched here, is intended for the implicit this parameter. Btw, the fact that You - a well-known expert in D - are asking, supports my thesis that DIP1000 needs a better representation, otherwise it likely won't receive much adoption.

Copy link
Member

@jmdavis jmdavis Feb 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I guess that without scope, the compiler thinks that a pointer to the this reference could be escaping or something? Yuck. If that's the case, we could end up having to slap scope on most member functions. Either way, I favor using it as little as possible until DIP 1000 is fully sorted out.

Btw, the fact that You - a well-known expert in D - are asking, supports my thesis that DIP1000 needs a better representation, otherwise it likely won't receive much adoption.

I doubt that most of us really understand DIP 1000 properly, though I expect that some do. It's complicated, and it's not fully implemented. I read through it previously but not recently, and glancing over it now, it looks more complicated than I recall it being. Either way, it's making my brain hurt looking at it now. The attribute soup is already arguably too complicated as it is.

Copy link
Contributor Author

@carblue carblue Feb 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read DIP1000.md a couple of times to have it somewhat stick in my brain (with all these different possible syntaxes for scope/return/ref +combinations) while I was working on PR #6041. It turned out - as Walter said in his 'pointers gone wild' talk - there where only a few places where I had to add scope for the implicit this parameter (i.e. not inferred currently; admittedly std.uni is heavily templated), but the remainder, Yes, needs slapping scope currently. There is often only 1 link of a chain missing and superfluous scope s can be removed again, as happened here.
And so far - while scanning phobos - I found only a few issues that separate us from -dip1000 compilable phobos; the memory corruption checks are better than their reputation; I created https://issues.dlang.org/show_bug.cgi?id=18444 in order to track missing implementation issues.

{
return _timezone.utcToTZ(_stdTime);
}
Expand All @@ -8990,7 +8990,7 @@ private:
/+
Converts the given hnsecs from $(LREF SysTime)'s time zone to std time.
+/
@property void adjTime(long adjTime) @safe nothrow
@property void adjTime(long adjTime) @safe nothrow scope
{
_stdTime = _timezone.tzToUTC(adjTime);
}
Expand Down