Syncronize CoordinateArraySequenceFactory::create signatures #46

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@manisandro

The overriding CoordinateArraySequenceFactory::create methods have different default parameter values than the create methods in the CoordinateSequenceFactory base class. This is potentially ambiguous, since

((CoordinateArraySequenceFactory*)factory)->create(0)

will fail to compile, while

((CoordinateSequenceFactory*)factory)->create(0)

will compile successfully.

The patch syncronizes the create signatures, and adds necessary

reinterpret_cast<std::vector<Coordinate> *>(0)

throughout the code to resolve the ambiguities.

@manisandro manisandro referenced this pull request in manisandro/libgeos Apr 2, 2015
@manisandro manisandro Add support for M values 681bdec
@strk
Member
strk commented Apr 10, 2015

I would like to avoid the reinterpret_cast. Would be maybe easier to expose a method taking no argument and meaning "empty sequence with automatically guessed dimensions"...

@manisandro

Ping?

@strk
Member
strk commented Apr 20, 2015

Works for me. Could you please add a test to tests/unit/geom/CoordinateArraySequenceFactoryTest.cpp for the new method,
remove empty spaces from blank lines (git diff --color shows them in red) and squash the commits ?

@manisandro

Ok, done.

@strk
Member
strk commented Apr 20, 2015

Reading the header, isn't there a conflict between these two methods of CoordinateArraySequenceFactory ?:

        CoordinateSequence *create(std::vector<Coordinate> *coords) const;
        CoordinateSequence *create(std::vector<Coordinate> *coords, std::size_t dims=0) const;

I guess that's the reason why there was no default value for "dims" before ?

@strk
Member
strk commented Apr 20, 2015

I guess the solution is dropping the single-argument version, to further sync with base class...

@manisandro

Yes indeed, good catch. The thing that is slightly ugly is that the implementation of the single-parameter version is

return new CoordinateArraySequence(coords,3);

i.e. as if the default parameter value for the dims variable was 3, whereas it is 0 in the two-parameter version. But in my opinion it makes sense to clean this up properly and hence yes, remove the single-parameter version.

@strk
Member
strk commented Apr 20, 2015
@manisandro

Tests still pass

@strk
Member
strk commented Apr 20, 2015

Pushed as r4052 (possibly 3a36e1a once it gets here)

@strk strk closed this Apr 20, 2015
@strk strk pushed a commit that referenced this pull request Apr 20, 2015
Sandro Santilli Cleanup CoordinateSequenceFactory interface
Adds method for creating empty sequence.
Syncronizes CoordinateArraySequenceFactory methods.

Patch by Sandro Mani,
see #46

git-svn-id: http://svn.osgeo.org/geos/trunk@4052 5242fede-7e19-0410-aef8-94bd7d2200fb
3a36e1a
@manisandro

Thanks

@manisandro manisandro deleted the manisandro:factory_params branch Apr 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment