On 24.04.2017 10:56, Cédric Champeau wrote:
Implementing equals/hashcode does not give you any order in a Set or
Map you can rely on. Really, I still don´t get what you target with
that. You require them in set to avoid duplicates.
I'm not talking about order here. I'm talking about reproducibility. A
linked hash set guarantees that things are read in the order they were
inserted. This is in general just enough. For example, we read in the
bytecode a list of interfaces, we add them to a set. If it's not linked,
then when we read them back, you have no guarantee. It has nothing to do
with _sorting_, which would reorder interfaces typically. The 2 uses
cases are different.
So we agree it is the "linked" ability that gives the order and equals
and hashcode are secondary
then don´t. Let´s change the API to not return Set.
Yes. I would say +100 if it wasn't for performance. Typically if the
algorithm needs to use `contains`, a `Set` is way more efficient.
I was saying to use List, because there is no equals and hashcode
defined. If they are not defined your search will be invalid unless, you
are searching using the exact same instance.
And... what hashcode function do you suggest for MethodNode and
ClassNode? Because I think they won't be cheap. If they are too
expensive "way more efficient" will become very different for small sets.
And
that's exactly why we use sets in lots of places. So to be clear, I did
a quick review of where we used sets and maps, and replaced that with
linked hash set and linked hash map when we needed both to guarantee
that the order of insertion is preserved (not an absolute order) *and*
that the algorithm required fast checks. This is all we need, I think.
But I'm calling for more than just a quick review, we should recheck all
our usage precisely. And avoid binary breaking changes, of course :)
as I said, maps are probably different, since there we most probably do
not use the AST instances as key, and if we did, it is probably a bug.
For the set cases.... well, maybe it would be better to give the
container better search abilities instead of exposing a special
collection here. That way we can ensure we have the implementation we need.
bye Jochen
