Skip to content
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

Add .& and .| #306

Merged
merged 1 commit into from
Jan 18, 2017
Merged

Add .& and .| #306

merged 1 commit into from
Jan 18, 2017

Conversation

nalimilan
Copy link
Member

These replace element-wise & and |, which are deprecated in Julia 0.6.
This syntax is not supported on 0.4 (raises a deprecation warning).

(Failure on nightlies is unrelated.)

@tkelman
Copy link
Contributor

tkelman commented Jan 14, 2017

can this work in an at-compat macro on 0.4?

@nalimilan
Copy link
Member Author

Not that I know of unfortunately. [1] .& [2] prints a deprecation warning on 0.4, even inside a macro.

@nalimilan
Copy link
Member Author

And even worse, it sometimes does not parse at all:

julia> X .& Y

WARNING: deprecated syntax "X .".
Use "X." instead.
ERROR: syntax: extra token "Y" after end of expression


# julia#17623
if VERSION >= v"0.5.0"
@eval quote # To work around unsupported syntax on Julia 0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

would at-static work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't exist on 0.4...

Copy link
Contributor

Choose a reason for hiding this comment

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

compat implements it

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, great. That works here, but not in the other file, since just parsing the syntax is an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that's what at-static was supposed to help with. and it's exported so shouldn't need at-compat to use

Copy link
Contributor

Choose a reason for hiding this comment

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

@static doesn't really help parsing error. It can avoid lowering error if something parses but raises error/depwarn during lowering or type inference.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the present case it fixes the error on 0.4, though the deprecation warnings remain.

# To work around unsupported syntax on Julia 0.4
parse("export .&, .|")
parse(".&(xs...) = broadcast(&, xs...)")
parse(".|(xs...) = broadcast(|, xs...)")
Copy link
Contributor

Choose a reason for hiding this comment

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

these will return Exprs, did you mean include_string ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, I got that totally wrong. Since I made the same mistake in tests, no wonder why they passed. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

at-static would look better here too

@nalimilan
Copy link
Member Author

Another question to address: the PR currently replaces .& with broadcast(&, ...) on 0.5, but it could equally replace it with &. Which is better? Cc: @stevengj

@nalimilan
Copy link
Member Author

Bump. This is needed to fix DataFrames on 0.6, among others.

@@ -1759,4 +1759,11 @@ if VERSION < v"0.5.0-dev+3669"
scale!(similar(A, promote_op(*, eltype(A), eltype(D.diag))), D.diag, A)
end

if VERSION >= v"0.5.0" && VERSION < v"0.6.0-dev.1614"
# To work around unsupported syntax on Julia 0.4
Copy link
Contributor

Choose a reason for hiding this comment

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

do we know what would have made these first parse?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't understand the question.

Copy link
Contributor

Choose a reason for hiding this comment

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

it was some pr during 0.5-dev that first made these parse, right? "parse more dot operators" by stevengj I want to say?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've made the check specific to that commit.

@stevengj
Copy link
Member

Would be nice if we could get CI working on nightly... I don't see a Compat issue for this?

These replace element-wise & and |, which are deprecated in Julia 0.6.
This syntax is not supported by Julia 0.4.
@nalimilan
Copy link
Member Author

AppVeyor failure due to network error.

@nalimilan nalimilan closed this Jan 18, 2017
@nalimilan nalimilan reopened this Jan 18, 2017
@nalimilan
Copy link
Member Author

Good to go now?

@ararslan ararslan merged commit e96a5e8 into master Jan 18, 2017
@ararslan ararslan deleted the nl/ops branch January 18, 2017 21:37
@@ -1759,4 +1759,11 @@ if VERSION < v"0.5.0-dev+3669"
scale!(similar(A, promote_op(*, eltype(A), eltype(D.diag))), D.diag, A)
end

if VERSION >= v"0.5.0-dev+5509" && VERSION < v"0.6.0-dev.1614"
Copy link
Contributor

Choose a reason for hiding this comment

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

how did you get the 1614 cutoff? I think that's wrong, I get 1632 at the merge commit b0a41531fc4a0345710375adb3d7f40b6220ae91

Copy link
Member Author

Choose a reason for hiding this comment

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

1614 corresponds to commit JuliaLang/julia@f216cbb, from JuliaLang/julia#17623 for which 1632 is the merge commit. Which is the most correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

The merge commit. Otherwise you're counting relative to where work on the PR branched off from, and there's going to be overlap for commits from master that don't have the feature. Linear numbering isn't quite sufficient to capture "newer or older than a given commit" so we should probably move towards feature tests where possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, see #318.

@stevengj
Copy link
Member

stevengj commented Feb 4, 2017

@nalimilan, & is probably faster on 0.5, but I don't think it's critical for Compat to use the fastest possible code on older releases.

tkelman added a commit to SciML/DiffEqPDEBase.jl that referenced this pull request Apr 5, 2017
worth noting the method definition of this as broadcast is implemented by Compat on 0.5
JuliaLang/Compat.jl#306
martinholters added a commit that referenced this pull request Aug 31, 2018
martinholters added a commit that referenced this pull request Aug 31, 2018
stevengj pushed a commit that referenced this pull request Sep 5, 2018
* Remove `take!(::Task)` definition for Julia versions prior to 0.6

Was added in #307.

* Remove `redirect_std*(f, stream)` definitions for Julia prior to v0.6

Were added in #275.

* Remove at-__DIR__ macro definition for Julia versions prior to 0.6

Was added in #281.

* Remove `broadcast` definition for equal-length tuples

Was added in #324 and #328

* Remove definitions of `unsafe_get` and `isnull` fallsback

Were added in #287.

* Remove defintions of `xor` and `⊻`

Were added in #289.

* Definitions of `numerator` and `denominator`

Were added in #290.

* Remove defintion of `iszero`

Was added in #305.

* Remove definition of `>:`

Was added in #336

* Remove definition of `take!(::Base.AbstractIOBuffer)`

Was added in #290.

* Remove definiton of `.&` and `.|`

Were added in #306.

* Remove definition of `Compat.isapprox`

Was added in #309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants