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

Don't initialize CoordinateSequence in constructor #765

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eyal0
Copy link
Contributor

@eyal0 eyal0 commented Dec 8, 2022

This ought to improve performance in the common case where initialization is unnecessary.

This fixes #764

@eyal0
Copy link
Contributor Author

eyal0 commented Dec 8, 2022

@dbaston Please take a look, thanks.

@eyal0 eyal0 marked this pull request as draft December 8, 2022 06:12
@eyal0 eyal0 marked this pull request as ready for review December 8, 2022 06:12
@eyal0 eyal0 force-pushed the patch-2 branch 5 times, most recently from d961c54 to 40be55a Compare December 8, 2022 06:43
This ought to improve performance in the common case where
initialization is unnecessary.

However, the `getDimension` method, in the case where the dimension is
unknown (0) looks at the z dimension of the first element in the array
to determine if there is z.  So we need to initialize at least the
first element.
@pramsey
Copy link
Member

pramsey commented May 4, 2023

@dbaston, is this a go or a no?

@dbaston
Copy link
Member

dbaston commented May 4, 2023

It makes me nervous despite seeing the tests pass. I would rather creation sites clearly indicate that they do not require initialization by using the constructor that provides this parameter.

@eyal0
Copy link
Contributor Author

eyal0 commented May 4, 2023

Does such a constructor exist?

I'm trying to remember why I found this issue in the first place. I suppose some unit test of mine found it. Maybe performance tanked or something?

@dbaston
Copy link
Member

dbaston commented May 4, 2023

Yes, see

/**
* Create a CoordinateSequence that packs coordinates of any dimension.
* Code using a CoordinateSequence constructed in this way must not
* attempt to access references to coordinates with dimensions that
* are not actually stored in the sequence.
*
* @param size size of the sequence to create
* @param hasz true if the stored
* @param hasm
* @param initialize
*/
CoordinateSequence(std::size_t size, bool hasz, bool hasm, bool initialize = true);

@robe2
Copy link
Member

robe2 commented Jan 10, 2024

This has been sitting here for over a year are we thinking about doing anything about it @dbaston @eyal0 @pramsey

@pramsey
Copy link
Member

pramsey commented Jan 10, 2024

Like @dbaston I hate mucking around in the initializers, because the knock-on effects are subtle. I defer to him as usual.

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

Successfully merging this pull request may close these issues.

Don't initialize CoordinateSequence in constructor
4 participants