Minor cleanups, style tweaks #20031

tkelman · 2017-01-14T12:15:33Z

Mostly removing empty lines right at the start of a block or right before end, consolidating zero-arg lets in many of the tests to save a few lines, and eval-ing a deprecation from #19901 into the right module (that commit is best viewed as f397cad?w=1, as is the last commit d5ea348?w=1 - or overall, https://github.com/JuliaLang/julia/pull/20031/files?w=1).

stevengj · 2017-01-14T13:17:16Z

test/dict.jl

@@ -651,7 +640,6 @@ Dict(1 => rand(2,3), 'c' => "asdf") # just make sure this does not trigger a dep
 end

 # issue 19995
-
 @test hash(Dict(Dict(1=>2) => 3, Dict(4=>5) => 6)) != hash(Dict(Dict(4=>5) => 3, Dict(1=>2) => 6))
 let a = Dict(Dict(3 => 4, 2 => 3) => 2, Dict(1 => 2, 5 => 6) => 1)
    b = Dict(Dict(1 => 2, 2 => 3, 5 => 6) => 1, Dict(3 => 4) => 2)


Looks like there is a comma missing on the previous line, also (so that b is a let variable like a, not that it matters much.)

Sacha0 · 2017-01-14T18:33:29Z

base/linalg/dense.jl

@@ -509,7 +509,7 @@ function logm(A::StridedMatrix)
        end
    end

-    if isreal(A) && ~np_real_eigs
+    if isreal(A) && !np_real_eigs


Sacha0 · 2017-01-14T18:37:41Z

base/multidimensional.jl

@@ -51,10 +51,14 @@ module IteratorsMD

    # arithmetic, min/max
    (-){N}(index::CartesianIndex{N}) = CartesianIndex{N}(map(-, index.I))
-    (+){N}(index1::CartesianIndex{N}, index2::CartesianIndex{N}) = CartesianIndex{N}(map(+, index1.I, index2.I))


Do these definitions substantially exceed the line length recommendation? These eyes find the existing (aligned) version easier to parse as a whole than the line-split version. Best!

113 columns

Sacha0 · 2017-01-14T18:51:30Z

doc/src/manual/control-flow.md

@@ -843,7 +843,8 @@ may have more values to generate and so might not yet be ready to return. With t
 and consumer can both run as long as they need to, passing values back and forth as necessary.

 Julia provides a [`Channel`](@ref) mechanism for solving this problem.
-A [`Channel`](@ref) is a waitable FIFO queue which can have multiple tasks reading and writing to it.
+A [`Channel`](@ref) is a waitable first-in first-out queue which can have
+multiple tasks reading and writing to it.


Perhaps "reading and" -> "reading from and"?

Sacha0 · 2017-01-14T18:57:08Z

test/arrayops.jl

@@ -904,8 +901,7 @@ end
 end

 @testset "single multidimensional index" begin
-    let


Is this let block redundant? (None of a, a2, b, or I occur in the enclosing scope?)

Sacha0 · 2017-01-14T19:00:11Z

test/arrayops.jl


 @testset "concatenations of dense matrices/vectors yield dense matrices/vectors" begin
-    let
-        N = 4
+    let N = 4


Likewise here, this let seems redundant with the @testset?

Sacha0 · 2017-01-14T19:02:59Z

test/complex.jl

@@ -208,8 +208,7 @@ end
 end

 @testset "log" begin
-    #  log(conj(z)) = conj(log(z))
-
+    # log(conj(z)) = conj(log(z))


Perhaps incorporate the comment in the testset description and nix the comment?

Sacha0 · 2017-01-14T19:03:40Z

test/complex.jl

@@ -242,8 +241,7 @@ end
 end

 @testset "exp" begin
-    #  exp(conj(z)) = conj(exp(z))
-
+    # exp(conj(z)) = conj(exp(z))


Likewise here, perhaps incorporate the comment in the testset description and nix the comment?

Sacha0 · 2017-01-14T19:04:22Z

test/complex.jl

@@ -278,8 +276,7 @@ end
 end

 @testset "expm1" begin
-    #  expm1(conj(z)) = conj(expm1(z))
-
+    # expm1(conj(z)) = conj(expm1(z))


Likewise here, perhaps incorporate the comment in the testset description and nix the comment?

Sacha0 · 2017-01-14T19:06:25Z

test/complex.jl

-    #  conj(x)^conj(y) = conj(x^y)
+    # equivalent to exp(y*log(x))
+    #   except for 0^0?
+    # conj(x)^conj(y) = conj(x^y)


Perhaps also a candidate for expanding the testset description and removing the comments?

Sacha0 · 2017-01-14T19:07:31Z

test/complex.jl

-    #  tanh(conj(z)) = conj(tanh(z))
-    #  tanh(-z) = -tanh(z)
+    # tanh(conj(z)) = conj(tanh(z))
+    # tanh(-z) = -tanh(z)


Likewise here, perhaps incorporate the comments in the testset description and nix the comments?

(Several more such candidates in this file, suppressing further review comments.)

Sacha0 · 2017-01-14T19:15:02Z

test/linalg/lu.jl

+    d = eltya == Int ? Tridiagonal(rand(1:7, n-1), rand(1:7, n), rand(1:7, n-1)) :
+        convert(Tridiagonal{eltya}, eltya <: Complex ?
+        Tridiagonal(complex.(dlreal, dlimg), complex.(dreal, dimg), complex.(dureal, duimg)) :
+        Tridiagonal(dlreal, dreal, dureal))


Perhaps indent the two Tridiagonal(... lines, making their nesting within the convert more obvious?

Sacha0 · 2017-01-14T19:17:10Z

test/linalg/lu.jl

-Hinv = Rational{BigInt}[(-1)^(i+j)*(i+j-1)*binomial(nHilbert+i-1,nHilbert-j)*binomial(nHilbert+j-1,nHilbert-i)*binomial(i+j-2,i-1)^2 for i = big(1):nHilbert,j=big(1):nHilbert]
+Hinv = Rational{BigInt}[(-1)^(i+j)*(i+j-1)*binomial(nHilbert+i-1,nHilbert-j)*
+    binomial(nHilbert+j-1,nHilbert-i)*binomial(i+j-2,i-1)^2
+    for i = big(1):nHilbert,j=big(1):nHilbert]


Perhaps indent the binomial(... and for(... lines further to clarify their part in the comprehension?

to where? it would start getting overly wide to align to the [

The [ was my thought, yes.

I'm not much of a fan of horizontal-alignment games like that, I'd rather just apply a simple set of rules that we could conceivably automate some day.

Sacha0 · 2017-01-14T19:18:35Z

test/parse.jl

@@ -384,6 +384,9 @@ end
 @test parse("x<:y<:z").head === :comparison
 @test parse("x>:y<:z").head === :comparison

+# reason PR #19765, <- operator, was reverted
+@test -2<-1 # DO NOT ADD SPACES


# DO NOT ADD SPACES 😆

especially right after a block opener and right before end

over 'environmental variable'

todo: make this a doctest

and linalg/dense.jl

remove lets where redundant with testsets move some comments into testset descriptions

ararslan · 2017-01-17T18:26:34Z

Is this good to go?

tkelman · 2017-01-17T23:54:47Z

As far as I'm concerned yes, though when merged it might cause conflicts with PR's that are messier to rebase than this would be.

tkelman · 2017-01-18T23:40:21Z

thanks for reviewing!

tkelman force-pushed the tk/various branch from 61f1eed to 030db03 Compare January 14, 2017 12:22

stevengj reviewed Jan 14, 2017

View reviewed changes

Sacha0 reviewed Jan 14, 2017

View reviewed changes

tkelman added 15 commits January 15, 2017 17:30

remove a bunch of unnecessary blank lines

3352ed1

especially right after a block opener and right before end

wrap long lines in test/linalg/lu.jl

cc982bd

wrap some long lines in base/multidimensional.jl

a4919d8

space try expression over multiple lines

a16015a

none of the other bullets here pretend to be sentences

fa30d63

Fix subj/verb agreement in broadcast docstring

9daf864

fix grammar in a comment

ded552a

Prefer 'environment variable' spelling

24e5afe

over 'environmental variable'

Spell out FIFO acronym in channel documentation

c214ac9

4 space indent control flow examples

1ddae06

don't shadow cmp function with a variable name in test/worlds.jl

15e8d11

add back test examples for isapprox

ca9e10b

todo: make this a doctest

Add a test that the <- operator from 19765 would have broken

1541b65

replace a let with a testset

73ee71b

use logical negation instead of bitwise in triangular.jl

ac96524

and linalg/dense.jl

tkelman added 5 commits January 15, 2017 17:30

can use deepcopy_internal without Base. qualification inside Base

05c8f76

eval Base.Test deprecations into right module

f397cad

do not indent preprocessor conditionals in codegen.cpp

3a4afe6

Save some vertical space with some previously zero-argument lets

e09ce52

Address review comments

d5ea348

remove lets where redundant with testsets move some comments into testset descriptions

tkelman force-pushed the tk/various branch from 030db03 to d5ea348 Compare January 16, 2017 01:55

stevengj approved these changes Jan 18, 2017

View reviewed changes

Sacha0 approved these changes Jan 18, 2017

View reviewed changes

tkelman merged commit 23c4fa4 into master Jan 18, 2017

tkelman deleted the tk/various branch January 18, 2017 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor cleanups, style tweaks #20031

Minor cleanups, style tweaks #20031

tkelman commented Jan 14, 2017 •

edited

Loading

stevengj Jan 14, 2017

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017

tkelman Jan 16, 2017

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017

Sacha0 Jan 14, 2017

tkelman Jan 16, 2017

Sacha0 Jan 16, 2017

tkelman Jan 16, 2017

Sacha0 Jan 14, 2017

ararslan commented Jan 17, 2017

tkelman commented Jan 17, 2017

tkelman commented Jan 18, 2017

Minor cleanups, style tweaks #20031

Minor cleanups, style tweaks #20031

Conversation

tkelman commented Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Sacha0 Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Jan 17, 2017

tkelman commented Jan 17, 2017

tkelman commented Jan 18, 2017

tkelman commented Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017 •

edited

Loading

Sacha0 Jan 14, 2017 •

edited

Loading