"someDouble = integralA / integralB" may indicate that the coder expects floating-point division

[cushon]

Original issue created by cpovirk@google.com on 2013-07-30 at 07:31 PM

---

(Similarly, "return integralA / integralB" from a double-returning method. Or either of those from a float-returning method. Or "(integralA / integralB) - someDouble." There are a lot of possibilities here, but even a naive check could find a lot of problems.)

The idea is to catch people who write something like "double seconds = millis / 1000" and expect to get a fractional number of seconds. That particular example likely isn't a big deal, as it's often used just for logging. But there are cases in which it can matter. I do see some scary cases in the Google codebase. For example, code will count the number of elements in a collection that match some predicate, and then it will compute "matches / collection.size()" to compare to a threshold. But with integer division, the result is always 0. I collected some of these cases into http://cl/50117990 for viewing inside Google. (There are plenty of other cases that involve time calculations. If I get ambitious, I could pull those out into their own CL.)

[cushon]

Original comment posted by cpovirk@google.com on 2013-07-30 at 08:00 PM

---

With the remaining Google-codebase instances (after I pulled out the especially buggy looking cases of <http://cl/50117990>), I've created two CLs:

Plenty of time-related examples in http://cl/50119899

Plenty of non-time-related examples in http://cl/50115656

[cushon]

Original comment posted by lowasser@google.com on 2013-07-30 at 08:21 PM

---

There are many cases in which this is desired behavior, though. Do you have any ideas on how to minimize false positives?

[cushon]

Original comment posted by cpovirk@google.com on 2013-07-30 at 08:25 PM

---

My impression from my search of the Google codebase is that this is rarely if ever desired. I do admit that it's frequently harmless.

Did you see specific cases that looked intentional? I guess you can IM/email me any links.

[cushon]

Original comment posted by lowasser@google.com on 2013-07-30 at 08:26 PM

---

I can think of several examples inside Guava that are deliberate. ;)

[cushon]

Original comment posted by cpovirk@google.com on 2013-07-30 at 08:31 PM

---

In that case, my idea of avoiding false positives is to write the check the way that I wrote it, as that apparently avoids matching any of the Guava occurrences :) The CLs above are the complete set of matches (other than the false positives that I sent you).

Where does this come up in Guava? Math stuff, I presume?

[cushon]

Original comment posted by lowasser@google.com on 2013-07-30 at 08:34 PM

---

Math stuff for certain; possibly Lists.partition and other related things that involve division.

[cushon]

Original comment posted by cpovirk@google.com on 2013-07-30 at 08:40 PM

---

Lists.partition appears to be OK. The output of its division is an integer, rather than a double, so this test wouldn't catch it. (Also, it wisely uses IntMath.divide.)

common.math is big enough that I'm too lazy to scan it all, but a naive search didn't turn up anything. Any specific pointers?

[cushon]

Original comment posted by kevinb@google.com on 2013-10-02 at 04:51 AM

---

I wonder if Louis is just saying that integer division in general is sometimes desired. It is, but the bug pattern proposed here is only for integer division expressions that are then implicitly casted to double or float.

In the rare case that it's desired behavior, is there a "natural" way to make it go away other than the ugly @SW? I was thinking that adding an explicit cast, like "return (double) (oneInt / anotherInt);" could be the way, but then the sad thing is that javac will warn.

[cushon]

Original comment posted by cpovirk@google.com on 2013-10-02 at 01:29 PM

---

The option I'd suggest would be:

int temp = oneInt / anotherInt; return temp;

But if people end up having to do that a lot, that's kind of a shame.

[cushon]

Original comment posted by cpovirk@google.com on 2013-10-02 at 01:31 PM

---

Another possibility is to use IntMath.divide, although that's kind of bulky.