Remove global const in init in cholmod #23449
Conversation
base/sparse/cholmod.jl
Outdated
| @@ -89,10 +88,9 @@ function __init__() | |||
| of CHOLMOD, or download the generic binaries | |||
| from www.julialang.org, which ship with the correct | |||
| versions of all dependencies. | |||
| """) | |||
There was a problem hiding this comment.
Why delete the empty lines and add these spaces? Not super important, but I think the previous behavior is nicer.
base/sparse/cholmod.jl
Outdated
|
|
||
| global const common_supernodal = | ||
| common_supernodal[] = |
There was a problem hiding this comment.
You can probably remove the convert.
There was a problem hiding this comment.
Yeah.
Thank you for the review and suggested improvements!
base/sparse/cholmod.jl
Outdated
| @@ -1330,7 +1338,7 @@ function cholfact!(F::Factor{Tv}, A::Sparse{Tv}; shift::Real=0.0) where Tv | |||
| cm = common() | |||
There was a problem hiding this comment.
Is it really necessary to have a function common() to get the global variable commonStruct?
There was a problem hiding this comment.
No, I don't think so. It used to take an argument but that was broken and got removed so I think we can just delete the function.
There was a problem hiding this comment.
@andreasnoack would you be opposed to renaming commonStruct to common_struct ? The cammelcase doesn't seem very commonly used in julia code.
|
This should be g2g now. perhaps run benchmarks? |
|
https://github.com/JuliaLang/julia/blob/master/test/codegen.jl#L51 |
|
Looks like #23365 |
|
tests all pass sans an unrelated error on appveyor. Should we run nano soldier here just to double check? |
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan |
|
G2G? |
|
@fredrikekre why? It wasn't four space aligned ..... |
|
The string was 4 space indented, you removed one space making the string 5 spaces indented so I added it back :) |
|
Please see line 100 and 116, it's now misaligned with those |
|
CI passed on the previous commit so should be ok to squash merge, and remove the ci skip in the process. |
|
That's not the point, the point is to have the indentation correct in the string, see https://docs.julialang.org/en/stable/manual/strings/#Triple-Quoted-String-Literals-1 |
|
I think we should not have indentation discussions. Or the author of any PR should be free to ignore anyone who points it out (unless they like the suggestion). |
cc @yuyichao @vtjnash