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.

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

Fix remaining import deprecation messages #4016

Merged
merged 2 commits into from Feb 22, 2016

Conversation

Projects
None yet
3 participants
@schveiguy
Member

schveiguy commented Feb 22, 2016

See #4015

This is the rest of them.

There are some places where it's not 100% clear whether the right call is a static import or a selective import.

This should be merged fairly quickly, because it touches a lot of files, the risk of this becoming conflicting is pretty high.

@MartinNowak

This comment has been minimized.

Member

MartinNowak commented Feb 22, 2016

Thanks a lot.

ab = (-lhs) ^^ this.re * exp(-PI * this.im);
ar = PI * this.re + log(-lhs) * this.im;
ab = (-lhs) ^^ this.re * std.math.exp(-std.math.PI * this.im);
ar = std.math.PI * this.re + std.math.log(-lhs) * this.im;

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

IMO, in math-related code such as std/complex.d it's only natural to use log and PI directly, instead of the ugly FQN's. What about making this one a selective import instead?

This comment has been minimized.

@MartinNowak

MartinNowak Feb 22, 2016

Member

Yeah import std.math : cos, exp, log, sin would be a bit nicer, in fact since hijacking of local symbols was fixed import std.math would be an option as well.

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

This was the trickiest file. Because std.complex has some of the exact same symbols as std.math

I asked myself the question, "what are the chances that std.complex will gain a definition for PI, log, etc.?", so I just went with the static import. There are other places I have no choice (example, sqrt).

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

I'll note that with new rules, the current module takes precedent over any imports, even scoped ones. So using selective imports may break later if these symbols get added.

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

Hmm, you're right, std.complex quite possibly may implement its own version of log, exp, etc.. But wouldn't they be adequately disambiguated by the argument type? Or would that still be a conflict under current rules?

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

The current file takes precedence I think, but not quite sure. So the compiler (I think) will stop looking for the symbol if it finds it in the current module, regardless of whether the overload would work elsewhere.

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

I'm wrong, I tested it out. Apparently, if you import selectively, that overrides any local module symbols. Is that the way it's supposed to work?

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

Update: yep, dlang.org specifies this behavior. A selective import binds it to the current namespace.

I'll work on re-fixing these.

immutable ab = r^^z.re * exp(-t*z.im);
immutable ar = t*z.re + log(r)*z.im;
immutable ab = r^^z.re * std.math.exp(-t*z.im);
immutable ar = t*z.re + std.math.log(r)*z.im;

This comment has been minimized.

@quickfur
@@ -388,6 +388,7 @@ struct Complex(T) if (isFloatingPoint!T)
ref Complex opOpAssign(string op, R)(R r)
if (op == "^^" && isFloatingPoint!R)
{
static import std.math;

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

ditto... even though arguably it's an annoyance to have to type out all the selected symbols for each import line. I dunno what's the best solution... a straight out import std.math runs the risk of (unintentional) hijacking. But still, I'm leaning towards keeping the mathematical expressions less verbose.

@@ -31663,7 +31663,7 @@ SysTime parseRFC822DateTime(R)(R value) @safe
}
// day-of-week
if(std.ascii.isAlpha(value[0]))
if(isAlpha(value[0]))

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

Nitpick: since you're at it, what about a space between if and (?

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

Honestly, no :) std.datetime has over 30k lines, and most of them are not this way. If you want to do the reformatting, I suggest we do it all at once, and only to std.datetime as a separate PR.

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

Fine, point taken. :-) Maybe it's time to reach for dfmt...

@@ -788,6 +791,7 @@ Complex!T sin(T)(Complex!T z) @safe pure nothrow @nogc
///
unittest
{
static import std.math;

This comment has been minimized.

@MartinNowak

This comment has been minimized.

@schveiguy

schveiguy Feb 22, 2016

Member

Hm... didn't notice this.

Is the correct fix to re-space the import statement, or to fix the asserts (they are only indented 2 spaces)?

This comment has been minimized.

@quickfur

quickfur Feb 22, 2016

Member

Re-space the asserts.

@quickfur

This comment has been minimized.

Member

quickfur commented Feb 22, 2016

Other than that, LGTM. Let's get this in sooner rather than later.

@schveiguy

This comment has been minimized.

Member

schveiguy commented Feb 22, 2016

Redid some of the static imports as selective, and fixed the assert spacing.

@quickfur

This comment has been minimized.

Member

quickfur commented Feb 22, 2016

LGTM

@quickfur

This comment has been minimized.

Member

quickfur commented Feb 22, 2016

Auto-merge toggled on

quickfur added a commit that referenced this pull request Feb 22, 2016

Merge pull request #4016 from schveiguy/fix_import_deprecations
Fix remaining import deprecation messages

@quickfur quickfur merged commit 0a885cd into dlang:master Feb 22, 2016

2 checks passed

CyberShadow/DAutoTest Documentation OK (25 additions, 9 deletions)
Details
auto-tester Pass: 10
Details

@schveiguy schveiguy deleted the schveiguy:fix_import_deprecations branch Feb 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment