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

BUG: Use enable_if with SFINAE to dispatch #1018

Conversation

Projects
None yet
4 participants
@blowekamp
Copy link
Member

commented Jun 13, 2019

addresses: #1003

@blowekamp blowekamp requested review from dzenanz and N-Dekker Jun 13, 2019

@dzenanz

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

I guess you now need to make it compile on the other compilers too 😄

@blowekamp blowekamp force-pushed the blowekamp:FixCastImageFilterMetaProgramming branch from 6893a50 to b8c401d Jun 13, 2019

@blowekamp blowekamp force-pushed the blowekamp:FixCastImageFilterMetaProgramming branch from b8c401d to 4bcff2c Jun 13, 2019

@blowekamp blowekamp changed the base branch from master to release Jun 13, 2019

@blowekamp

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

This patch should be merged into the release and master branches. It has been rebased onto the release branch.

@blowekamp blowekamp requested review from hjmjohnson and thewtex Jun 14, 2019

@blowekamp blowekamp closed this Jun 14, 2019

@blowekamp blowekamp reopened this Jun 14, 2019

@blowekamp

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

@agravgaard Can you please test this patch with your version of ICC? Thanks.

@agravgaard

This comment has been minimized.

Copy link

commented Jun 15, 2019

It looks like your enable_if solution works.

I only get some seemingly unrelated linking errors now:

7>ITKIOTransformInsightLegacy-5.0.lib(itkTxtTransformIOFactory.obj) : error LNK2019: unresolved external symbol "protected: __cdecl itk::TxtTransformIOTemplate<double>::TxtTransformIOTemplate<double>(void)" (??0?$TxtTransformIOTemplate@N@itk@@IEAA@XZ) referenced in function "public: virtual class itk::SmartPointer<class itk::LightObject> __cdecl itk::CreateObjectFunction<class itk::TxtTransformIOTemplate<double> >::CreateObject(void)" (?CreateObject@?$CreateObjectFunction@V?$TxtTransformIOTemplate@N@itk@@@itk@@UEAA?AV?$SmartPointer@VLightObject@itk@@@2@XZ)
7>C:\Users\au451932\Projects\itk5-mod\build\bin\RelWithDebInfo\ITKIOTransformInsightLegacyTestDriver.exe : fatal error LNK1120: 2 unresolved externals
8>ITKIOTransformMatlab-5.0.lib(itkMatlabTransformIOFactory.obj) : error LNK2019: unresolved external symbol "protected: __cdecl itk::MatlabTransformIOTemplate<float>::MatlabTransformIOTemplate<float>(void)" (??0?$MatlabTransformIOTemplate@M@itk@@IEAA@XZ) referenced in function "public: virtual class itk::SmartPointer<class itk::LightObject> __cdecl itk::CreateObjectFunction<class itk::MatlabTransformIOTemplate<float> >::CreateObject(void)" (?CreateObject@?$CreateObjectFunction@V?$MatlabTransformIOTemplate@M@itk@@@itk@@UEAA?AV?$SmartPointer@VLightObject@itk@@@2@XZ)
8>ITKIOTransformMatlab-5.0.lib(itkMatlabTransformIOFactory.obj) : error LNK2019: unresolved external symbol "protected: __cdecl itk::MatlabTransformIOTemplate<double>::MatlabTransformIOTemplate<double>(void)" (??0?$MatlabTransformIOTemplate@N@itk@@IEAA@XZ) referenced in function "public: virtual class itk::SmartPointer<class itk::LightObject> __cdecl itk::CreateObjectFunction<class itk::MatlabTransformIOTemplate<double> >::CreateObject(void)" (?CreateObject@?$CreateObjectFunction@V?$MatlabTransformIOTemplate@N@itk@@@itk@@UEAA?AV?$SmartPointer@VLightObject@itk@@@2@XZ)
8>C:\Users\au451932\Projects\itk5-mod\build\bin\RelWithDebInfo\ITKIOTransformMatlabTestDriver.exe : fatal error LNK1120: 2 unresolved externals
6>itkIOTransformHDF5Test.obj : error LNK2019: unresolved external symbol "protected: __cdecl itk::HDF5TransformIOTemplate<double>::HDF5TransformIOTemplate<double>(void)" (??0?$HDF5TransformIOTemplate@N@itk@@IEAA@XZ) referenced in function "public: static class itk::SmartPointer<class itk::HDF5TransformIOTemplate<double> > __cdecl itk::HDF5TransformIOTemplate<double>::New(void)" (?New@?$HDF5TransformIOTemplate@N@itk@@SA?AV?$SmartPointer@V?$HDF5TransformIOTemplate@N@itk@@@2@XZ)
6>ITKIOTransformHDF5-5.0.lib(itkHDF5TransformIOFactory.obj) : error LNK2001: unresolved external symbol "protected: __cdecl itk::HDF5TransformIOTemplate<double>::HDF5TransformIOTemplate<double>(void)" (??0?$HDF5TransformIOTemplate@N@itk@@IEAA@XZ)
6>itkIOTransformHDF5Test.obj : error LNK2019: unresolved external symbol "protected: __cdecl itk::HDF5TransformIOTemplate<float>::HDF5TransformIOTemplate<float>(void)" (??0?$HDF5TransformIOTemplate@M@itk@@IEAA@XZ) referenced in function "public: static class itk::SmartPointer<class itk::HDF5TransformIOTemplate<float> > __cdecl itk::HDF5TransformIOTemplate<float>::New(void)" (?New@?$HDF5TransformIOTemplate@M@itk@@SA?AV?$SmartPointer@V?$HDF5TransformIOTemplate@M@itk@@@2@XZ)
6>ITKIOTransformHDF5-5.0.lib(itkHDF5TransformIOFactory.obj) : error LNK2001: unresolved external symbol "protected: __cdecl itk::HDF5TransformIOTemplate<float>::HDF5TransformIOTemplate<float>(void)" (??0?$HDF5TransformIOTemplate@M@itk@@IEAA@XZ)
6>C:\Users\au451932\Projects\itk5-mod\build\bin\RelWithDebInfo\ITKIOTransformHDF5TestDriver.exe : fatal error LNK1120: 2 unresolved externals```
@dzenanz

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@blowekamp should this be merged now?

@agravgaard

This comment has been minimized.

Copy link

commented Jun 23, 2019

Update: The linking errors does not occur when building with BUILD_SHARED_LIBS=ON. So it's "just" an export problem.

With BUILD_SHARED_LIBS=ON it compiles and all tests, except 6 passes:

  • At least 3 of those failed because I use the TBB multithreader (the OldPool/OldPlatform tests) - probably not caused by the intel compiler.
  • 2 of them, vnl_algo_test_qr and vnl_algo_test_svd results in what looks like an underflow in a few subtests, i.e. large numbers or inf where 0 expected.
  • The last is itkFastChamferDistanceImageFilterTest4 were the "inner positive/negative points" are both 24 from the expected and the "rest of points" are 148 off.

I personally don't care about getting these tests passing - just wanted to let you know.

Also BUILD_SHARED_LIBS=ON is fine with me, on Windows. So no rush on getting it fixed.

@thewtex
Copy link
Member

left a comment

Note: macOS / Python warnings can be ignored

@blowekamp

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Yes, this patch is ready to go. This commit is for both the release and the master branches. I created PR #1036 for the master branch.

It's not clear to me the current best practice to get a patch/commit into the release branch. I just have two separate PR for the same commit now, targeting the two separate branches.

@dzenanz

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

With respect to longer term maintainability, it looks like the best way is to merge a commit into release, then merge release branch into master. That way there should be no merge conflict on rebase, now or in the future - at least nearer future.

If a commit is merged separately into release and master branch, it gets counted twice as @N-Dekker recently noticed with 5.0.0 release notes.

@dzenanz dzenanz merged commit 0c7d03f into InsightSoftwareConsortium:release Jun 24, 2019

7 of 11 checks passed

ITK.Linux in progress
Details
ITK.Windows in progress
Details
ITK.macOS in progress
Details
ci/circleci CircleCI is running your tests
Details
CDash Build analysis summary
Details
ITK.Linux.Python #20190614.4 succeeded
Details
ITK.Windows.Python #20190614.4 succeeded
Details
ITK.macOS.Python #20190614.4 succeeded
Details
WIP Ready for review
Details
ghostflow-check-master ghostflow-check-master
Details
ghostflow-check-release ghostflow-check-release
Details
@blowekamp

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

If a commit is merged separately into release and master branch, it gets counted twice as @N-Dekker recently noticed with 5.0.0 release notes.

This is not accurate. If a same commit is merged into both, it only gets counted once. On the other hand if a commit is merged into master, then rebased onto release such that it is another commit is created the came changes with a new hash will be merged. However it is relatively hard to ensure that the same commit is merged with Github, this was one of the nice advantages of the Kitware stage repo/process.

The downside of merging directly into release, is that we don't get the nightly testing on the merged commit before merging in to the more stable release branch.

🤷‍♂

@blowekamp

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Thanks for merging. This also requires a PR for release being merged into master.

@dzenanz Already took care of that.

@dzenanz

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

The downside of merging directly into release, is that we don't get the nightly testing on the merged commit before merging in to the more stable release branch.

@thewtex do you think we should go the route of first merging to master, waiting a night or two, then merging to release? And possibly merging release into master after that, to reduce changes of merge conflicts in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.