fix compile in itk reader/writer #1153

kerautret · 2016-03-25T21:16:12Z

PR Description

fix compile in itk reader/writer

Checklist

[n.a] Unit-test of your feature with Catch.
[n.a] Doxygen documentation of the code completed (classes, methods, types, members...)
[n.a] Documentation module page added or updated.
New entry in the ChangeLog.md added.
No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
All continuous integration tests pass (Travis & appveyor)

dcoeurjo · 2016-03-26T21:38:11Z

src/DGtal/io/readers/ITKReader.ih

@@ -64,7 +64,8 @@ namespace DGtal {
    const Domain& domain = dgtal_itk_image.domain();

    typedef ConstImageAdapter<DGtalITKImage, Domain, functors::Identity, Value, Functor> AdaptedImage;
-    const AdaptedImage adapted(dgtal_itk_image, domain, functors::Identity(), aFunctor);
+    const functors::Identity identityFunctor{};


I apply the same change of @rolanddenis ;) looks fine without with my compilo but I suppose that there a reason.

😄
In that case, it is exactly the same as without {}: it calls the constructor without argument. It is a new syntax from C++11. It enhances the construction of object and, especially, it allows me to replace a common mistake that I make:
const functors::Identity identityFunctor();
by a valid syntax:
const functors::Identity identityFunctor{};

Well, I would have preferred to get the () removed instead of something that can be ambiguous for non-c11 dev.
(Not sure to see why this new syntax enhance the construction.. I'll have to read the doc;))

dcoeurjo · 2016-03-26T21:38:40Z

beside my minor comment, this PR is fine.
Thanks

dec cosmetic fix

fixing extension removal

kerautret · 2016-03-28T13:23:48Z

ok @dcoeurjo I have remove the {} since @rolanddenis confirms that in this case it is equivalent.
Perhaps we can merge and change other {} in other PR? (there also some in DGTalTools)

dcoeurjo · 2016-03-28T13:32:31Z

Yes I agree. Thanks

dcoeurjo · 2016-03-28T13:32:54Z

(Just waiting for Travis ;))

… with test adds

rolanddenis · 2016-03-31T17:16:11Z

Don't merge yet: do you have try to compile it with clang ? It should fail because of the missing {}.
I can't try this specific file (my ITK is not compiled with C++11) but this simple example:

#include "DGtal/base/BasicFunctors.h"
int main()
{
  const DGtal::functors::Identity id;
  return 0;
}

failed with clang++ and it is not a bug.
(explanation coming)

rolanddenis · 2016-03-31T18:38:11Z

There is a small rule when using default initialization with constant object (§8.5/7):

If a program calls for the default initialization of an object of a const-qualified type T, T shall be a class type with a user-provided default constructor.

It seems that this rule exists to avoid invalid initialization of POD class with const qualifier.
In order to compile, there is many solutions:

declaring a user-provided default constructor. "user-provided" means that the constructor is not set to the default (with = default at the end of the first declaration). Therefore, the class becomes non-POD.
removing the const qualifier (against the const correctness).
explicitly using the "value initialization" (instead of the "default initialization") by adding {}:
const DGtal::functors::Identity id{};
or, before C++11:
const DGtal::functors::Identity id = DGtal::functors::Identity();

gccdoes not complain because it implements a proposed change in the draft, coming after the C++11standard that weaken this rule for class without members (see http://stackoverflow.com/questions/26077807/why-does-gcc-allow-a-const-object-without-a-user-declared-default-constructor-bu and http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#253 ). This change seems to be still not applied in the last draft.

Refs:

kerautret · 2016-03-31T19:24:59Z

thanks @rolanddenis , yes you are right it fails, perhaps I wrongly switch branch by testing by removing it, sorry ;( !
So I revert to the initial good version ;)

dcoeurjo · 2016-03-31T19:40:54Z

Thanks for the details! I've learned a lot of things.
So what should we do? Should we recommend to add default constructors to our functors or let the user deal with the {} (or its pre-c++11 version) ?

kerautret · 2016-03-31T19:45:34Z

perhaps the default constructor is better but we will lost the POD of Identity, but I don't know if it is a problem to be no more Plain Old Data ?

rolanddenis · 2016-03-31T20:10:29Z

I found that strange to provide a default constructor that do nothing in a class that owns nothing (and that could have only static methods). If using {} is really a problem, I prefer removing the const. A maybe better solution is to mix the old fashion way with auto:
const auto id = DGtal::functors::Identity();
?

kerautret · 2016-03-31T21:03:02Z

Perhaps the {} is fine from the user point of view, since the message of Clang is really explicit:

and also the auto should be fine..

dcoeurjo · 2016-04-01T08:36:20Z

Ok, you're both right. Let's keep the {} for const instanciations.
Thanks for the discussion 😄

So, can i merge this branch or you need to revert your edits, Bertrand ?

kerautret · 2016-04-01T08:37:44Z

I revert it just now, I was waiting the end of discussion ;)

…rior3D Fix fill interior exterior 3d

dcoeurjo · 2016-04-01T08:39:32Z

🆗

kerautret · 2016-04-01T08:41:04Z

done

README page update (license)

* fix empty faces in align axis tubular direction and better optimisation on face association (approx hausdorff distance instead using direction) * add visual test * complete testMesh * Changelog * fix typo test and better test nb faces * clean in test * typo

kerautret · 2016-04-05T08:19:28Z

@dcoeurjo can we merge?

dcoeurjo · 2016-04-05T08:53:37Z

Yep merging. Thanks

fix compile in itk reader/writer

kerautret · 2016-04-05T09:01:46Z

thanks! ;)

Le 5 avr. 2016 à 10:53, David Coeurjolly notifications@github.com a écrit :

Yep merging. Thanks

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

dcoeurjo · 2016-04-05T10:35:17Z

ok.. there is an issue.. could you please merge the master back ?

kerautret · 2016-04-05T11:10:48Z

it looks like it was just a meriging bug ! ;)

Le 5 avr. 2016 à 12:35, David Coeurjolly notifications@github.com a écrit :

ok.. there is an issue.. could you please merge the master back ?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

elcerdo and others added 11 commits March 16, 2016 15:55

select vector field color

98859d1

fix useless asserts

837dd1a

oups

8787afc

fix 3d viewer add cone

63c8f46

disable eigen deprecated warning with cpp11

a6541ae

fix compile in itk reader/writer

bda20a5

change log

a02506e

fixing extension removal

14850c5

Changelog edit

8b19eef

testing

da49df7

minor typo

232193c

dcoeurjo reviewed Mar 26, 2016
View reviewed changes

dcoeurjo added the waiting author feedbacks label Mar 26, 2016

dcoeurjo and others added 2 commits March 26, 2016 21:40

Merge pull request DGtal-team#1148 from elcerdo/DECCosmetics

b79323b

dec cosmetic fix

Merge pull request DGtal-team#1154 from dcoeurjo/FixObjFilename

53116bf

fixing extension removal

dcoeurjo self-assigned this Mar 28, 2016

remove {} in functor def

d315f77

kerautret added 4 commits March 29, 2016 10:47

Fix fill interior/exterior in 3D with open or closed KhalimskySpaceND…

f8ade3b

… with test adds

add test for 3D exterior fill

1e9f0a4

complement test in 2D

42a0a30

change log

2613aad

Merge pull request DGtal-team#1156 from kerautret/FixFillInteriorExte…

5b8a335

…rior3D Fix fill interior exterior 3d

back {} ;)

628eb94

dcoeurjo and others added 5 commits April 3, 2016 18:53

README page update (license)

a47a7ab

Update README.md

e8bab36

missing verb

fe9125f

Merge pull request DGtal-team#1158 from dcoeurjo/readme

ed28393

README page update (license)

Fix tubular mesh

49e90eb

* fix empty faces in align axis tubular direction and better optimisation on face association (approx hausdorff distance instead using direction) * add visual test * complete testMesh * Changelog * fix typo test and better test nb faces * clean in test * typo

Merge pull request DGtal-team#1153 from kerautret/FixITKReader

1e55cc2

fix compile in itk reader/writer

kerautret closed this Apr 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix compile in itk reader/writer #1153

fix compile in itk reader/writer #1153

kerautret commented Mar 25, 2016

dcoeurjo Mar 26, 2016

kerautret Mar 26, 2016

rolanddenis Mar 27, 2016

dcoeurjo Mar 28, 2016

dcoeurjo commented Mar 26, 2016

kerautret commented Mar 28, 2016

dcoeurjo commented Mar 28, 2016

dcoeurjo commented Mar 28, 2016

rolanddenis commented Mar 31, 2016

rolanddenis commented Mar 31, 2016

kerautret commented Mar 31, 2016

dcoeurjo commented Mar 31, 2016

kerautret commented Mar 31, 2016

rolanddenis commented Mar 31, 2016

kerautret commented Mar 31, 2016

dcoeurjo commented Apr 1, 2016

kerautret commented Apr 1, 2016

dcoeurjo commented Apr 1, 2016

kerautret commented Apr 1, 2016

kerautret commented Apr 5, 2016

dcoeurjo commented Apr 5, 2016

kerautret commented Apr 5, 2016

dcoeurjo commented Apr 5, 2016

kerautret commented Apr 5, 2016

Navigation Menu

fix compile in itk reader/writer #1153

fix compile in itk reader/writer #1153

Conversation

kerautret commented Mar 25, 2016

PR Description

Checklist

dcoeurjo Mar 26, 2016

Choose a reason for hiding this comment

kerautret Mar 26, 2016

Choose a reason for hiding this comment

rolanddenis Mar 27, 2016

Choose a reason for hiding this comment

dcoeurjo Mar 28, 2016

Choose a reason for hiding this comment

dcoeurjo commented Mar 26, 2016

kerautret commented Mar 28, 2016

dcoeurjo commented Mar 28, 2016

dcoeurjo commented Mar 28, 2016

rolanddenis commented Mar 31, 2016

rolanddenis commented Mar 31, 2016

kerautret commented Mar 31, 2016

dcoeurjo commented Mar 31, 2016

kerautret commented Mar 31, 2016

rolanddenis commented Mar 31, 2016

kerautret commented Mar 31, 2016

dcoeurjo commented Apr 1, 2016

kerautret commented Apr 1, 2016

dcoeurjo commented Apr 1, 2016

kerautret commented Apr 1, 2016

kerautret commented Apr 5, 2016

dcoeurjo commented Apr 5, 2016

kerautret commented Apr 5, 2016

dcoeurjo commented Apr 5, 2016

kerautret commented Apr 5, 2016