Redundant code with isMultipolygon()

[GerdP]

The method isMultipolygon() is implemented in IPrimitive (return false) and (only) IRelation overrides it:

@Override
default boolean isMultipolygon() {
    return "multipolygon".equals(get("type")) || 
}

Still, we have many places where something like

e.osm instanceof Relation && ((Relation) e.osm).isMultipolygon()

is used instead of

e.osm.isMultipolygon()

What is the reason for this double check? It seems to be a waste of time and space as the java compiler doesn't remove the redundant check.
Even worse seems this code in MultipolygonCache:

    private static boolean isMultipolygon(OsmPrimitive p) {
        return p instanceof Relation && ((Relation) p).isMultipolygon();
    }

I work on a patch to remove all this redundant code, please stop me if I missed something.

[GerdP]

Oops, forgot that instanceof also is a "not null" check, so the replacement code would be

e.osm != null && e.osm.isMultipolygon()

[GerdP]

Maybe a problem occurs when the primitive is an instance of RelationData (which implements IRelation).
So, the instanceof check cannot be simply removed but the cast is not needed.
BTW: I fear I've written a lot of code which would doesn't work with an IRelation that is not also a Relation :(

[stoecker]

My guess: These checks come from a time, where isMultipolygon() was only implemented in Relation?

[GerdP]

It was implemented in OsmPrimitive and Relation before r13667. Anyway, my idea doesn't work well. The instanceof check is in fact needed in many places because the code would crash with RelationData. Open question is if that code works at all when the primitves are not instances of OsmPrimitve?
Example: ConditionFactory.closed() will always return false when e.osm is not a Way or Relation

        static boolean closed(Environment e) { // NO_UCD (unused code)
            if (e.osm instanceof Way && ((Way) e.osm).isClosed())
                return true;
            return e.osm instanceof Relation && ((Relation) e.osm).isMultipolygon();
        }

So I have the gut feeling that the changes in r13810 were not yet used to render non-OSM data.

[Don-vip]

Replying to GerdP:

I have the gut feeling that the changes in r13810 were not yet used to render non-OSM data.

It was. The use case was to render ​atlas files with ​atlas plugin. But I didn't check all JOSM possibilities, maybe some parts like this one are not working.

[Don-vip]

In 15122/josm:

see #17763 - use interfaces where possible

[GerdP]

Seems that atlas data doesn't use multipolygons?

[Don-vip]

The data model is different, I don't remember how MPs are handled. Do you want to do something more in this ticket, or can we close it?

[GerdP]

I think my proposed changes would not improve much and may cause trouble, so yes, let's close it. I think wontfix is correct then?