chore: Minor cleanups & dead code deletion #24920
Conversation
| } | ||
|
|
||
| /** Produces a string index, which is a list of `sections`, i.e | ||
| /** Produces a string index, which is a list of `sections`, i.e., |
There was a problem hiding this comment.
this was one of my PhD advisor's pet peeves, I've been conditioned to fix it 😬
|
|
||
| package object tools { | ||
|
|
||
| /** Cached single-element list of Nil. (Whether this helps performance has not been tested) */ |
There was a problem hiding this comment.
It's not just for performance, but easier to read and write. I miss literalUnit if I have to write Literal(Constant(())).
Also see your other comment that makes a performance claim.
There was a problem hiding this comment.
Easier than Nil :: Nil yeah, but compared to List(Nil) ?
| val content = source.content() | ||
| val (start, end) = | ||
| if content.lift(span.end - 1).exists(_ == '`') then | ||
| if content.lift(span.end - 1).contains('`') then |
There was a problem hiding this comment.
These are not generally the same because of type-unsafe contains. I might get a compiler warning on ==.
There was a problem hiding this comment.
Ah, I misread the direction of that generic constraint... is there a reason why contains does this? (I know Java has a lot of this kind of behavior)
|
|
||
| import core.Types.WildcardType | ||
|
|
||
| /** Common values hoisted out for performance */ |
There was a problem hiding this comment.
I see the performance claim was here.
| val e = if msg == null then AssertionError() else AssertionError("assertion failed: " + msg) | ||
| e.setStackTrace(Array()) | ||
| throw e | ||
|
|
There was a problem hiding this comment.
this caught my eye the other day for some reason! LOC have unseen costs if it makes you pause.
| targetScript: String = "", | ||
| compiler: Boolean = false, | ||
| quiet: Boolean = false, | ||
| colors: Boolean = false, |
There was a problem hiding this comment.
I would not know if this is used anywhere (in a build tool), but I would be cautious. It used to be easier to know "how is the compiler launched".
There was a problem hiding this comment.
If it is used, we'll get some feedback early enough, right? I asked around for who uses MainGenericCompiler and was told other tools mostly call the compiler directly as a library.
|
Thanks, I enjoy tidying! My general comment would be that "absolutizing" imports is only noise. I would avoid touching Removing all unused imports is strictly worse. I think |
Fair enough, I was trying to better understand which files depend on which modules and found it easier to see which imports are stdlib vs the compiler's own stuff, but I can revert. |
SolalPirelli
force-pushed
the
solal/cleanup
branch
3 times, most recently
from
258f33f to
ad3bd74
Compare
SolalPirelli
force-pushed
the
solal/cleanup
branch
from
ad3bd74 to
64eb66e
Compare
4 participants
I was looking through some compiler files to understand what's going on, and noticed some stuff that could be cleaned up.
Some of this is just stuff IntelliJ highlighted, like typos and dead imports.