vigna / fastutil

fastutil extends the Java™ Collections Framework by providing type-specific maps, sets, lists and queues.
Apache License 2.0
1.8k stars 197 forks source link

Fix warnings #182

Closed techsy730 closed 3 years ago

techsy730 commented 3 years ago

Fix a bunch of warnings.

By my compilation settings (which may differ from Vigna's), this reduces compilation warnings from 971 to 46.

vigna commented 3 years ago

OK, now I'm at 0 Eclipse warnings and 29 compiler warnings. Some varargs, some ambiguous calls. Let's look into the latter, they make lambdas really painful...

vigna commented 3 years ago

I have made all type-specific forEach() named forEachInt(), etc. This makes it possible to write lambdas without casting.

techsy730 commented 3 years ago

The change to the forEach broke a bunch of stuff expecting to override the old forEach(PrimitiveConsumer) method (thank you @Override for catching this). I can fix this up, but was the original really causing issues in practice? I never had an issue with it being ambiguous when I was giving it lambdas.

In any case, if we keep them a separate name, we can get rid of the ugly overloads with

     * <p><b>WARNING:</b> Overriding this is almost always a mistake, as this
     * overload only exists to disambiguate. Instead override the {@code forEach()} overload
     * that uses the JDK's primitive consumer type (e.g. {@link java.util.function.IntConsumer}).
     * <br>If Java supported final default methods, this would be one, but sadly it does not.
     * <p>If you checked and are overriding the version with {@code java.util.function.XConsumer}, and
     * still see this warning, then your IDE is incorrectly conflating this method with the proper
     * method to override, and you can safely ignore this message.

As there is no longer an overload with both a primitive and a object based consumer. This also means we can get rid of the shim classes that only are there to declare them as final.

techsy730 commented 3 years ago

One downside to having a separate named method is if someone were to use IntCollection.forEach(i -> / some int action /), it would now go through a boxing/unboxing cycle instead of choosing IntConsumer. Yes they will get a deprecation warning for it but it could still slip under the radar.

vigna commented 3 years ago

I know. I didn't realize, and I'm rushing to fix. I hoped nobody would notice. LOL.

So, usually if the compiler says it's ambiguous, it is. If there are two ways to interpret a lambda, you have to do something like casting. We ran into this for another series of methods (andThen?) and the problem was real.

I mean, if you have a forEach() method taking an IntConsumer and one taking a Consumer<? super Integer>, which one the compiler can choose for a trivial lambda x-> {} ? I can't believe this was compiling, but I might try.

vigna commented 3 years ago

One downside to having a separate named method is if someone were to use IntCollection.forEach(i -> / some int action /), it would now go through a boxing/unboxing cycle instead of choosing IntConsumer. Yes they will get a deprecation warning for it but it could still slip under the radar.

This is completely standard in fastutil. It is like get() and getInt().

techsy730 commented 3 years ago

I was writing things like IntCollection.forEach(i -> a.b *= i) just fine. I think what may have been saving me was the existence of the Fastutil IntConsumer overload, which extends both the JDK primitive and the JDK object based consumers. And thus it was choosing the "most specific" of all of them.

Definitely not something we should be relying on though.

EDIT: And I was in the middle of fixing this too, shall I wait for your change and then I can clean up the messy overload that isn't needed anymore once that is done?

vigna commented 3 years ago

Indeed, all the ambiguity warnings are about classes that are not int, long, or double. But creating a double standard will be very confusing IMHO.

Yeah, please wait for me, I'll try my best even if the hierarchy is staggering.

vigna commented 3 years ago

OK, I deleted everything. I have to think this thoroughly. forEachRemaining(KEY_ITERATOR) is inherited from PrimitiveIterator, so we cannot change it.

vigna commented 3 years ago

I'm really tired now—will be back on it tomorrow, and check ambiguity more carefully.

vigna commented 3 years ago

So, you're right. Apparently the existence of a common subtype forces the choice of that subtype (empirically, I did not check the JLS). What I wrote was wrong—warnings are there for all classes. So you can go on with what you are doing, let's touch base tomorrow.

There are still some varargs warnings around (javac).

techsy730 commented 3 years ago

I'm pretty close to fixing the forEach compilation issues and the cleanup of the now unuseful overloads, want a PR?

techsy730 commented 3 years ago

Ok, got it: https://github.com/vigna/fastutil/pull/186

vigna commented 3 years ago

The other thing is real: List.replaceAll() cannot lambda. I'd use something replaceAllInts(), even if it means some more defines.

vigna commented 3 years ago

The remaining warnings are varargs-related warning that shouldn't be there, as there are annotations (Eclipse indeed does not report problems). So once replaceAll() is fixed I think we can consider the warnings part OK.

vigna commented 3 years ago

I fixed replaceAll(): now all type-specific instances have a type suffix, e.g., replaceAllShorts().

vigna commented 3 years ago

So, just to understand: you didn't put together for replaceAll() the trick you pulled with forEach() because you did not think it was worth the effort?

techsy730 commented 3 years ago

More that I didn't realize removeIf had the issue too. I don't have the potential ambiguous overload warning on and I wasn't able to find that option.

Should we have a removeIf that accepts a fastutil consumer?

vigna commented 3 years ago

Mmmhhh... I don't have the warning either. Are you sure that there's a problem there?

Maybe it's worth implementing the trick for replaceable, too. I like when I can keep the original names.

vigna commented 3 years ago

BTW, besides Eclipse (or whatever IDE you're using) different options, we compile with -Xlint:all, which might explain why you don't see some of the warnings I see.

vigna commented 3 years ago

You're right, same problem with removeIf(). I will never understand why they didn't make IntPredicate extend Predicate. That's so nonsense.

Wouldn't also Iterators.any()/Iterators.all() etc. have the same problem? I mean, basically anything that uses a JDK primitive something.

vigna commented 3 years ago

OK, I added a type-specific Predicate class following exactly the logic of Consumer. Now we should look around for places where we can avoid ambiguity.

techsy730 commented 3 years ago

I filed https://github.com/vigna/fastutil/issues/197 for discussion so we don't have to keep discussion going on a closed PR, which is a bit tricky to keep track of.

vigna commented 3 years ago

Great idea.