From f47c9b7e490b8fcc48a29cca7219d8c6b41c45a1 Mon Sep 17 00:00:00 2001 From: ScottPJones Date: Sat, 20 Jun 2015 23:39:48 -0400 Subject: [PATCH] Improve error messages Updated messages based on review Update variable names per Matt Updated tests to handle ArgumentError instead of BoundsError when passed iterable More specific exceptions for test_throws Fix dangling right parenthesis Update non-abstract array error messages and tests --- base/abstractarray.jl | 137 +++++++++++++++++++++++++++--------------- test/copy.jl | 14 +++-- 2 files changed, 100 insertions(+), 51 deletions(-) diff --git a/base/abstractarray.jl b/base/abstractarray.jl index b09d3331dd27a..9df0466466f0d 100644 --- a/base/abstractarray.jl +++ b/base/abstractarray.jl @@ -62,7 +62,12 @@ ndims{T<:AbstractArray}(::Type{T}) = ndims(super(T)) length(t::AbstractArray) = prod(size(t))::Int endof(a::AbstractArray) = length(a) first(a::AbstractArray) = a[1] -first(a) = (s = start(a); done(a, s) ? throw(ArgumentError("collection must be non-empty")) : next(a, s)[1]) + +function first(itr) + state = start(itr) + done(itr, state) && throw(ArgumentError("collection must be non-empty")) + next(itr, state)[1] +end last(a) = a[end] function stride(a::AbstractArray, i::Integer) @@ -154,11 +159,13 @@ checkbounds(A::AbstractArray, I::AbstractVector{Bool}) = length(A) == length(I) checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}) = (@_inline_meta; _checkbounds(length(A), I) || throw_boundserror(A, I)) function checkbounds(A::AbstractMatrix, I::Union{Real,AbstractArray,Colon}, J::Union{Real,AbstractArray,Colon}) @_inline_meta - (_checkbounds(size(A,1), I) && _checkbounds(size(A,2), J)) || throw_boundserror(A, (I, J)) + (_checkbounds(size(A,1), I) && _checkbounds(size(A,2), J)) || + throw_boundserror(A, (I, J)) end function checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}, J::Union{Real,AbstractArray,Colon}) @_inline_meta - (_checkbounds(size(A,1), I) && _checkbounds(trailingsize(A,Val{2}), J)) || throw_boundserror(A, (I, J)) + (_checkbounds(size(A,1), I) && _checkbounds(trailingsize(A,Val{2}), J)) || + throw_boundserror(A, (I, J)) end @generated function checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}...) meta = Expr(:meta, :inline) @@ -197,7 +204,7 @@ similar( a::AbstractArray, T, dims::Dims) = Array(T, dims) function reshape(a::AbstractArray, dims::Dims) if prod(dims) != length(a) - throw(ArgumentError("dimensions must be consistent with array size")) + throw(ArgumentError("dimensions must be consistent with array size (expected $(length(a)), got $(prod(dims)))")) end copy!(similar(a, dims), a) end @@ -216,11 +223,11 @@ end # if src is not an AbstractArray, moving to the offset might be O(n) function copy!(dest::AbstractArray, doffs::Integer, src) - doffs < 1 && throw(BoundsError()) + doffs < 1 && throw(BoundsError(dest, doffs)) st = start(src) i, dmax = doffs, length(dest) @inbounds while !done(src, st) - i > dmax && throw(BoundsError()) + i > dmax && throw(BoundsError(dest, i)) val, st = next(src, st) dest[i] = val i += 1 @@ -228,20 +235,28 @@ function copy!(dest::AbstractArray, doffs::Integer, src) return dest end +# copy from an some iterable object into an AbstractArray function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer) if (doffs < 1) | (soffs < 1) - throw(BoundsError()) + doffs < 1 && throw(BoundsError(dest, doffs)) + throw(ArgumentError(string("source start offset (",soffs,") is < 1"))) end st = start(src) for j = 1:(soffs-1) - done(src, st) && throw(BoundsError()) + if done(src, st) + throw(ArgumentError(string("source has fewer elements than required, ", + "expected at least ",soffs,", got ",j-1))) + end _, st = next(src, st) end dn = done(src, st) - dn && throw(BoundsError()) + if dn + throw(ArgumentError(string("source has fewer elements than required, ", + "expected at least ",soffs,", got ",soffs-1))) + end i, dmax = doffs, length(dest) @inbounds while !dn - i > dmax && throw(BoundsError()) + i > dmax && throw(BoundsError(dest, i)) val, st = next(src, st) dest[i] = val i += 1 @@ -252,15 +267,20 @@ end # this method must be separate from the above since src might not have a length function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer, n::Integer) - n < 0 && throw(BoundsError()) + n < 0 && throw(BoundsError(dest, n)) n == 0 && return dest dmax = doffs + n - 1 if (dmax > length(dest)) | (doffs < 1) | (soffs < 1) - throw(BoundsError()) + doffs < 1 && throw(BoundsError(dest, doffs)) + soffs < 1 && throw(ArgumentError(string("source start offset (",soffs,") is < 1"))) + throw(BoundsError(dest, dmax)) end st = start(src) for j = 1:(soffs-1) - done(src, st) && throw(BoundsError()) + if done(src, st) + throw(ArgumentError(string("source has fewer elements than required, ", + "expected at least ",soffs,", got ",j-1))) + end _, st = next(src, st) end i = doffs @@ -269,7 +289,7 @@ function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer, n::Inte dest[i] = val i += 1 end - i <= dmax && throw(BoundsError()) + i <= dmax && throw(BoundsError(dest, i)) return dest end @@ -278,9 +298,7 @@ end function copy!(dest::AbstractArray, src::AbstractArray) n = length(src) - if n > length(dest) - throw(BoundsError()) - end + n > length(dest) && throw(BoundsError(dest, n)) @inbounds for i = 1:n dest[i] = src[i] end @@ -290,16 +308,21 @@ end function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray) copy!(dest, doffs, src, 1, length(src)) end + function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray, soffs::Integer) - soffs > length(src) && throw(BoundsError()) + soffs > length(src) && throw(BoundsError(src, soffs)) copy!(dest, doffs, src, soffs, length(src)-soffs+1) end -function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray, soffs::Integer, n::Integer) - n < 0 && throw(BoundsError()) + +function copy!(dest::AbstractArray, doffs::Integer, + src::AbstractArray, soffs::Integer, + n::Integer) n == 0 && return dest - if soffs+n-1 > length(src) || doffs+n-1 > length(dest) || doffs < 1 || soffs < 1 - throw(BoundsError()) - end + n < 0 && throw(BoundsError(src, n)) + soffs+n-1 > length(src) && throw(BoundsError(src, soffs+n-1)) + doffs+n-1 > length(dest) && throw(BoundsError(dest, doffs+n-1)) + doffs < 1 && throw(BoundsError(dest, doffs)) + soffs < 1 && throw(BoundsError(src, soffs)) @inbounds for i = 0:(n-1) dest[doffs+i] = src[soffs+i] end @@ -308,9 +331,15 @@ end copy(a::AbstractArray) = copy!(similar(a), a) -function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int}) - if length(ir_dest) != length(ir_src) || length(jr_dest) != length(jr_src) - throw(ArgumentError("source and destination must have same size")) +function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, + A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int}) + if length(ir_dest) != length(ir_src) + throw(ArgumentError(string("source and destination must have same size (got ", + length(ir_src)," and ",length(ir_dest),")"))) + end + if length(jr_dest) != length(jr_src) + throw(ArgumentError(string("source and destination must have same size (got ", + length(jr_src)," and ",length(jr_dest),")"))) end checkbounds(B, ir_dest, jr_dest) checkbounds(A, ir_src, jr_src) @@ -326,9 +355,15 @@ function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{ return B end -function copy_transpose!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int}) - if length(ir_dest) != length(jr_src) || length(jr_dest) != length(ir_src) - throw(ArgumentError("source and destination must have same size")) +function copy_transpose!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, + A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int}) + if length(ir_dest) != length(jr_src) + throw(ArgumentError(string("source and destination must have same size (got ", + length(jr_src)," and ",length(ir_dest),")"))) + end + if length(jr_dest) != length(ir_src) + throw(ArgumentError(string("source and destination must have same size (got ", + length(ir_src)," and ",length(jr_dest),")"))) end checkbounds(B, ir_dest, jr_dest) checkbounds(A, ir_src, jr_src) @@ -690,7 +725,7 @@ function hcat{T}(A::AbstractVecOrMat{T}...) for j = 1:nargs Aj = A[j] if size(Aj, 1) != nrows - throw(ArgumentError("number of rows must match")) + throw(ArgumentError("number of rows of each array must match (got $(map(x->size(x,1), A)))")) end dense &= isa(Aj,Array) nd = ndims(Aj) @@ -722,7 +757,7 @@ function vcat{T}(A::AbstractMatrix{T}...) ncols = size(A[1], 2) for j = 2:nargs if size(A[j], 2) != ncols - throw(ArgumentError("number of columns must match")) + throw(ArgumentError("number of columns of each array must match (got $(map(x->size(x,2), A)))")) end end B = similar(full(A[1]), nrows, ncols) @@ -761,11 +796,13 @@ function cat_t(catdims, typeC::Type, X...) for i = 2:nargs for d = 1:ndimsC currentdim = (d <= ndimsX[i] ? size(X[i],d) : 1) - if dims2cat[d]==0 - dimsC[d] == currentdim || throw(DimensionMismatch("mismatch in dimension $(d)")) - else + if dims2cat[d] != 0 dimsC[d] += currentdim catsizes[i,dims2cat[d]] = currentdim + elseif dimsC[d] != currentdim + throw(DimensionMismatch(string("mismatch in dimension ",d, + " (expected ",dimsC[d], + " got ",currentdim,")"))) end end end @@ -777,7 +814,8 @@ function cat_t(catdims, typeC::Type, X...) offsets = zeros(Int,length(catdims)) for i=1:nargs - cat_one = [ dims2cat[d]==0 ? (1:dimsC[d]) : (offsets[dims2cat[d]]+(1:catsizes[i,dims2cat[d]])) for d=1:ndimsC] + cat_one = [ dims2cat[d] == 0 ? (1:dimsC[d]) : (offsets[dims2cat[d]]+(1:catsizes[i,dims2cat[d]])) + for d=1:ndimsC ] C[cat_one...] = X[i] for k = 1:length(catdims) offsets[k] += catsizes[i,k] @@ -817,9 +855,8 @@ typed_hcat(T::Type, A::AbstractArray...) = cat_t(2, T, A...) function hvcat(nbc::Integer, as...) # nbc = # of block columns n = length(as) - if mod(n,nbc) != 0 - throw(ArgumentError("all rows must have the same number of block columns")) - end + mod(n,nbc) != 0 && + throw(ArgumentError("number of arrays $n is not a multiple of the requested number of block columns $nbc")) nbr = div(n,nbc) hvcat(ntuple(i->nbc, nbr), as...) end @@ -850,16 +887,16 @@ function hvcat{T}(rows::Tuple{Vararg{Int}}, as::AbstractMatrix{T}...) Aj = as[a+j-1] szj = size(Aj,2) if size(Aj,1) != szi - throw(ArgumentError("mismatched height in block row $(i)")) + throw(ArgumentError("mismatched height in block row $(i) (expected $szi, got $(size(Aj,1)))")) end if c-1+szj > nc - throw(ArgumentError("block row $(i) has mismatched number of columns")) + throw(ArgumentError("block row $(i) has mismatched number of columns (expected $nc, got $(c-1+szj))")) end out[r:r-1+szi, c:c-1+szj] = Aj c += szj end if c != nc+1 - throw(ArgumentError("block row $(i) has mismatched number of columns")) + throw(ArgumentError("block row $(i) has mismatched number of columns (expected $nc, got $(c-1))")) end r += szi a += rows[i] @@ -875,12 +912,12 @@ function hvcat{T<:Number}(rows::Tuple{Vararg{Int}}, xs::T...) a = Array(T, nr, nc) if length(a) != length(xs) - throw(ArgumentError("argument count does not match specified shape")) + throw(ArgumentError("argument count does not match specified shape (expected $(length(a)), got $(length(xs)))")) end k = 1 @inbounds for i=1:nr if nc != rows[i] - throw(ArgumentError("row $(i) has mismatched number of columns")) + throw(ArgumentError("row $(i) has mismatched number of columns (expected $nc, got $(rows[i]))")) end for j=1:nc a[i,j] = xs[k] @@ -907,7 +944,7 @@ function typed_hvcat(T::Type, rows::Tuple{Vararg{Int}}, xs::Number...) nc = rows[1] for i = 2:nr if nc != rows[i] - throw(ArgumentError("row $(i) has mismatched number of columns")) + throw(ArgumentError("row $(i) has mismatched number of columns (expected $nc, got $(rows[i]))")) end end len = length(xs) @@ -1031,7 +1068,7 @@ function sub2ind{T<:Integer}(dims::Tuple{Vararg{Integer}}, I::AbstractVector{T}. for i=1:M indices[i] += s*(Ij[i]-1) end - s*= (j <= N ? dims[j] : 1) + s *= (j <= N ? dims[j] : 1) end return indices end @@ -1125,7 +1162,10 @@ function map(f, iters...) nxtvals = cell(len) cont = true for idx in 1:len - done(iters[idx], states[idx]) && (cont = false; break) + if done(iters[idx], states[idx]) + cont = false + break + end end while cont for idx in 1:len @@ -1133,7 +1173,10 @@ function map(f, iters...) end push!(result, f(nxtvals...)) for idx in 1:len - done(iters[idx], states[idx]) && (cont = false; break) + if done(iters[idx], states[idx]) + cont = false + break + end end end result diff --git a/test/copy.jl b/test/copy.jl index 338d386137537..0a047802dd079 100644 --- a/test/copy.jl +++ b/test/copy.jl @@ -21,14 +21,20 @@ for (dest, src, bigsrc, emptysrc, res) in [ @test copy!(copy(dest), 99, src(), 99, 0) == dest @test copy!(copy(dest), 1, emptysrc()) == dest - @test_throws BoundsError copy!(dest, 1, emptysrc(), 1) + x = emptysrc() + exc = isa(x, AbstractArray) ? BoundsError : ArgumentError + @test_throws exc copy!(dest, 1, emptysrc(), 1) - for idx in [0, 4] + for idx in (0, 4) @test_throws BoundsError copy!(dest, idx, src()) @test_throws BoundsError copy!(dest, idx, src(), 1) @test_throws BoundsError copy!(dest, idx, src(), 1, 1) - @test_throws BoundsError copy!(dest, 1, src(), idx) - @test_throws BoundsError copy!(dest, 1, src(), idx, 1) + x = src() + exc = isa(x, AbstractArray) ? BoundsError : ArgumentError + @test_throws exc copy!(dest, 1, x, idx) + x = src() + exc = isa(x, AbstractArray) ? BoundsError : ArgumentError + @test_throws exc copy!(dest, 1, x, idx, 1) end @test_throws BoundsError copy!(dest, 1, src(), 1, -1)