Warnings Cleanup

[tadhunt]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Documentation
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:

Problem

Cleanup lots of warnings

• GitHub issue (optional):

Solution

Action

[garbagemule]

There's just one curly brace in a wrong spot, and then some unused imports removal that belongs in a different commit. The rest is fine and ready for merge, I think.

[garbagemule]

There's something weird about the curlies here. I think line 356 just needs to be "dedented" by one indent level because it belongs to the switch statement and not the default branch.

[tadhunt]

Fixed in eb8cfac

[garbagemule]

These belong in the unused import commit.

[tadhunt]

ok, I moved this into a new commit

[garbagemule]

Whoops, I missed some of the other commits, one sec...

[garbagemule]

I think nuking EntityPosition would be great. We can remove the "unused" warning suppression once I get a few test cases that use the remaining methods into the test suite.

[garbagemule]

In IntelliJ, the registerConstant(), registerUnaryOperator(), and registerBinaryOperator() methods are all marked as "unused", which is why these suppressions are here.

Turns out it doesn't produce warnings for the other methods, because these are used in the test suite. I think I will try to buff out the test suite a bit to include tests of these methods as well in order to back up the removal of the suppress annotations.

[garbagemule]

I had no idea this class existed, wtf...

I think we can safely remove the entire class since it isn't actually used anywhere. I don't think it has ever been in use, because the commit that introduces it (back in 2011) doesn't even use it.

[tadhunt]

OK, do you want me to get rid of it in this PR or do you want to do that in a separate PR?

[garbagemule]

I'll just remove it on the master branch. I think you should try to amend the commit that removed this line, then we won't have issues with rebasing on top of master.

[garbagemule]

These warnings are a little hyper-aggressive, but I suppose there is an ounce of rationale to their paranoia. The actual issue is that the underlying interfaces, UnaryOperation and BinaryOperation, inherit from the generic Function and BiFunction interfaces in the java.util.function package. Because these interfaces have generic type parameters, and because generics in Java don't support primitive types, the two operation interfaces must use Double instead of double. The warnings, then, say that e.g. Math::sqrt(double) requires an unchecked conversion (or unboxing) of the Double that the interface is given into the double that the sqrt function requires. This could potentially lead to null pointer exceptions, since you could technically pass a null value to a UnaryOperation object. Of course, this never actually happens, but the compiler can't know that.

There are two better ways around the warnings:

1. Absolve the UnaryOperation and BinaryOperation interfaces of their dependence on Function and BiFunction and explicitly define methods double apply(double) and double apply(double, double), respectively, in them.
2. Switch to a lambda function like the one used for other operators, e.g. the unary +/- or the binary ^ operator.

The latter doesn't actually solve the underlying problem, because a null reference could still be passed to a UnaryOperation or a BinaryOperation. I'm not sure why VS Code doesn't continue to warn about it, but the current proposal doesn't actually solve the underlying problem either, as the new methods still take Double arguments instead of double (which they must with the current interfaces).

The former is definitely the way to go. I probably decided to use the Function and BiFunction interfaces from the standard library in an effort to try to "be correct" about it, but it doesn't actually provide any benefit whatsoever. If you're up for it, give it a shot! If not, I can work it in :)