#17763 closed enhancement (wontfix)
Redundant code with isMultipolygon()
Reported by: | GerdP | Owned by: | team |
---|---|---|---|
Priority: | minor | Milestone: | |
Component: | Core | Version: | |
Keywords: | Cc: |
Description
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.
Attachments (0)
Change History (12)
comment:1 by , 5 years ago
comment:2 by , 5 years ago
Milestone: | → 19.05 |
---|---|
Priority: | normal → minor |
Type: | task → enhancement |
comment:3 by , 5 years ago
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 :(
comment:4 by , 5 years ago
My guess: These checks come from a time, where isMultipolygon() was only implemented in Relation?
follow-up: 6 comment:5 by , 5 years ago
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.
comment:6 by , 5 years ago
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.
comment:9 by , 5 years ago
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?
comment:10 by , 5 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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?
comment:11 by , 5 years ago
Resolution: | fixed → wontfix |
---|
comment:12 by , 5 years ago
Milestone: | 19.05 |
---|
Oops, forgot that instanceof also is a "not null" check, so the replacement code would be