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

Minor edit #3

Merged
merged 3 commits into from
Sep 7, 2016
Merged

Minor edit #3

merged 3 commits into from
Sep 7, 2016

Conversation

dcoeurjo
Copy link

@dcoeurjo dcoeurjo commented Sep 6, 2016

Hop

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 6, 2016

Here you have my issues with Xcode.. weird stuff..

line 222, testVoronoiMap, the IDE wants me to add extra {} around false, false and later complains with the VoronoiMap constructor (not viable constructor).

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 6, 2016

BTW, you never use isPeriodic()

Compared to the previous version, there is a slight overhead due to the if (isPeriodic[dim]) tests if I'm right.

/**
* Disabling default constructor.
*/
VoronoiMap() = delete;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, C++ doesn't implicitly declare the default constructor if any other constructor is defined. So deleting it is not mandatory even if it is clearer.

@rolanddenis
Copy link
Owner

About the error with Xcode, I think it should accept the brace elision but since it is a common pitfall, I propose to add the extra {}:

 bool testVoronoiMap( std::array<bool, 2> const& periodicity = { { false, false } } )

Yes, I designed isPeriodic() for user-interface only but I should used it instead of testing myPeriodicitySpec directly.

I'm agree that the only overhead is on the periodicity tests since I specifically split the loops in generic and periodic parts. It should be negligible, isn't it ?

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 6, 2016

Yeah it's negligible.
An alternative would have been to consider a static periodicity map (in template parameters) allowing the compiler to optimize the code (and cut branches) at compile time but it would probably have been less interesting for the user.

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 6, 2016

BTW, it was not an error from Xcode, just a warning.. my bad

@rolanddenis
Copy link
Owner

Yes, I also think about it but since other DGtal classes don't work this way (actually only KhalimskySpaceND 😉 ), it would complicate user life. However, with additional work, it is possible to have a class that works both way (like Eigen with Dynamic magic value).

@rolanddenis
Copy link
Owner

Can I merge ?

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 7, 2016

Yep please. Try to merge the commits without closing my PR (and don't know if the PR will still be open with no diff...).
I'll probably help you to polish the doc according to this new cool feature.

@rolanddenis rolanddenis merged commit e3097cd into rolanddenis:periodicVoronoiMap Sep 7, 2016
@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 7, 2016

;) damit, it automatically closes the PR

@rolanddenis
Copy link
Owner

😄

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 7, 2016

ok I 've just pushed a tiny edit and i have to create a new PR so that you can see it...

@rolanddenis
Copy link
Owner

Ok, next time, I will directly merge from you branch ...

I will also push some documentation updates.

@dcoeurjo
Copy link
Author

dcoeurjo commented Sep 7, 2016

👌

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.

2 participants