-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Slight VersionSet refactoring, test intersect(::VersionSet, ::VersionSet)
#20598
Conversation
carlobaldassi
commented
Feb 13, 2017
- Enforce VectorSet normalization in constructor (fixes equality testing and generally makes more sense)
- Improve VectorSet normalization code (faster, fewer allocations),
- Test VectorSet intersections (including the empty cases which were at the root of issue Failure in pkg tests #20566)
base/pkg/types.jl
Outdated
end | ||
if lo < up | ||
# The "fusing" check has the only purpose to avoid | ||
# extra allocations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd word this as "The only purpose of the fusing check is to avoid extra allocations"
base/pkg/types.jl
Outdated
struct VersionSet | ||
intervals::Vector{VersionInterval} | ||
VersionSet(intervals::Vector{VersionInterval}) = new(normalize!(intervals)) | ||
Base.copy(vset::VersionSet) = new(copy(vset.intervals)) # avoids normalize! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why isn't this defined outside the type block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid going through normalize!
by calling new
directly (the idea being that vset.intervals
is already normalized, being an invariant of the type, so we can safely just copy it and save time). It's the equivalent of a copy constructor of C++, except that I think this is more idiomatic Julia (I had initially written VersionSet(::VersionSet)
, but it didn't seem right).
base/pkg/types.jl
Outdated
end | ||
|
||
show(io::IO, s::VersionSet) = join(io, s.intervals, " ∪ ") | ||
show(io::IO, s::VersionSet) = isempty(s) ? print(io, "∅") : join(io, s.intervals, " ∪ ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neither ∅ nor ∪ display properly in the windows console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to do here. ∪ was already there, but now it's more exposed to the user through the backtrace printing. Should I set them to empty
and U
on Windows? E.g.:
const _empty_symbol = @static is_windows() ? "empty" : "∅"
const _union_symbol = @static is_windows() ? " U " : " ∪ "
show(io::IO, s::VersionSet) = isempty(s) ? print(io, _empty_symbol) :
join(io, s.intervals, _union_symbol)
Other suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest printing VersionSet(∅ empty)
/ VersionSet(union of v"1.1.1" ∪ v"1.1.2" ∪ ...)
everywhere. This helps both terminals that can't interpret unicode, and people who don't recognize it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems way too verbose, considering that it's going to be shown to users in backtrace error messages like these:
version range [0.0.0,0.2.0) ∪ [0.3.0,∞) required by package SomePackage, whose allowed version range is [0.1.0,0.1.2) ∪ [0.1.4,∞):
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could just say "or"
s/intersetct/intersect/ |
intersetct(::VersionSet, ::VersionSet)
intersect(::VersionSet, ::VersionSet)
* Enforce VectorSet normalization in constructor (fixes equality testing) * Improve VectorSet normalization code (faster, fewer allocations), * Test VectorSet intersections (including the empty cases which were at the root of issue #20566) * Change VectorSet printing to avoid unicode on Windows
4c8f57f
to
9079707
Compare
Rebased and addressed comments. |