Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CoordinateSequence constructors change dimension to 3, fail to set hasZ #1090

Open
tfiner opened this issue May 3, 2024 · 2 comments
Open

Comments

@tfiner
Copy link

tfiner commented May 3, 2024

  1. GEOS_COORDSEQ_PADZ is looked at in 3 constructors in CoordinateSequence.cpp, each time ignoring the hasz parameter, then heavy handedly bumping the m_stride manually to 1 more than the constructor's signature would indicate. The constructors also all fail to account for what they have done and don't set m_hasz(true). This has a cascading effect where the sequences then incorrectly report their number of dimensions. And it makes the type returned (XYZ) out of sync with hasZ too.

  2. CoordinateSequence(std::size_t sz, std:size_t dim): adds a 1 to dim, regardless of GEOS_COORDSEQ_PADZ, but then also doesn't set m_hasz(true).

The only constructor that doesn't lie is CoodinateSequence(const std::initializer_list<CoordinateXYZM>& list) which correctly sets all the members as expected.

It is surprising to me that the type system isn't being used here to greater effect. It is a bit surprising that the underlying data is being laid out manually (XYZ), and then it is wasting space. One of the reasons to choose C++ over other languages is that in C++ we don't pay for things we aren't using. From my own experience, I use 2D algorithms far more often than 3D and I suspect others using this library are as well.

@dbaston
Copy link
Member

dbaston commented May 3, 2024

See #872 for discussion about why this is the case.

@tfiner
Copy link
Author

tfiner commented May 3, 2024

I understand why you added it, I'm saying that to me, the it is inconsistent to allocate for Z, then say it doesn't have Z, return XYZ as a type, and set the stride to 3. 2 out of 4 things say that the coordinates have 3 dimensions: hasZ(), getDimension() vs. getCoordinateType() and stride(). Admittedly, stride() is private, but so was that comment in the code for the define that made everything XYZ.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants