review: refactor: reference comparison between List and Set

[SirYwell]

When the underlying data structure in CtIntersectionTypeReferenceImpl was changed (#659), one reference check wasn't changed. From what I've seen that didn't introduce any behavioral changes other than the parameters passed to onListDeleteAll.

I didn't find any contracts for that method, that's why I consider it to be a refactoring instead of a bug fix. If that's wrong, please let me know.

[slarse]

Hi @SirYwell, and welcome to Spoon!

There's no doubt that what you found here is a bug. However, as you suggest, fixing it doesn't actually change any behavior. I think the reason for that is that the entire if statement is just redundant, as setBounds defers to addBound, which duplicates the behavior. There's also no particular requirement that a mutable list must be passed to onListDeleteAll, so I don't see why we need to create one.

To me, it looks like you could just entirely remove the affected if-statement without changing behavior. WDYT?

[slarse]

On a completely unrelated side note, I spotted another bug here: onListDeleteAll is not called if setting bounds to an empty list or null, related to #3953. Just a note to myself.

[SirYwell]

Indeed, that would work too. I wasn't sure about the onListDeleteAll, that's why I chose the solution that aligned more with the mentioned PR. I'll change it in a bit.

On your side note: I saw that too and randomly checked another place where it is used. I ended up in CtEnumImpl#setEnumValues, where it isn't called when passing null but it is called when passing an empty list.

[slarse]

LGTM, will merge after 24 hour wait to let other integrators have a say.

I saw that too and randomly checked another place where it is used. I ended up in CtEnumImpl#setEnumValues, where it isn't called when passing null but it is called when passing an empty list.

It's, let's say, wildly inconsistent. There's currently nothing enforcing the way listeners are called, and so it's really hard to make it all consistent. We're looking into how we can shape it up to be more consistent.

Oh, and a small request from my part: avoid force pushing when you have received feedback on your PR. It's a bit confusing as a reviewer to have previously reviewed commits disappear :)

[slarse]

@SirYwell Congrats on your first contribution!

If you enjoy refactoring, there's lots of room for improvement throughout Spoon to improve the general code quality. You can pretty much open any random class and find plenty of things to improve.

Remove dead or redundant code is of course the most satisfying refactoring there is, so nice work on finding this one :)