HairyFotr / linter

Static Analysis Compiler Plugin for Scala
Apache License 2.0
268 stars 34 forks source link

[false positive] `These two ifs can be merged into one` with nested ifs (?) #9

Closed jrudolph closed 11 years ago

jrudolph commented 11 years ago

There are two legitimately nested if so not sure what this is about:

[warn] /home/johannes/git/telfish/spray/spray/spray-can/src/main/scala/spray/can/server/StatsSupport.scala:45: These two ifs can be merged into one.
[warn]       if (currentMoc > moc)
[warn]       ^

Here's the complete method:

@tailrec
final def adjustMaxOpenConnections(): Unit = {
  val co = connectionsOpened.get
  val cc = connectionsClosed.get
  val moc = maxOpenConnections.get
  val currentMoc = co - cc
  if (currentMoc > moc)
    if (!maxOpenConnections.compareAndSet(moc, currentMoc)) adjustMaxOpenConnections()
}
HairyFotr commented 11 years ago

This one is more of a style check... the two if could really be merged into one.

I agree that sometimes it's more clear to write it as two ifs.

jrudolph commented 11 years ago

Ah, I completely missed what that message was about. Maybe the message can be changed into: These nested ifs can be merged into one.

I wouldn't worry to much about what is style and what is a more severe warning but rather make it configurable (I know it's on your list...).

HairyFotr commented 11 years ago

Fixed the wording, and thanks a lot for all the feedback! I'll get to the other issues soon. :+1:

BTW, one more thing that is done already, but isn't documented anywhere - you can ignore warnings with a //nolint comment at the end of the line. But that's only for when you disagree only with a particular instance of a warning - if you disagree with the whole check, it's better to wait for the configuration options, or to wait for a fix, if it's a false positive.