From: Sean M. <se...@ro...> - 2012-09-21 18:42:49
Attachments:
vxl-C++11-ITK.patch
|
Hi all, Attached is a patch against svn ToT incorporating this fix from ITK: <http://review.source.kitware.com/#/c/6025/> It fixes many warnings from clang. Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Sean M. <se...@ro...> - 2012-09-28 19:26:14
Attachments:
vxl-math2.patch
|
On Fri, 21 Sep 2012 14:42:39 -0400, Sean McBride said: >Attached is a patch against svn ToT incorporating this fix from ITK: ><http://review.source.kitware.com/#/c/6025/> > >It fixes many warnings from clang. Recent changes to svn meant that this patch no longer applied. New patch attached. Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Sean M. <se...@ro...> - 2012-10-09 14:59:23
|
Ping. Anyone take a look at this patch? It's pretty straightforward, and working already in ITK... Cheers, Sean On Fri, 28 Sep 2012 15:26:04 -0400, Sean McBride said: >On Fri, 21 Sep 2012 14:42:39 -0400, Sean McBride said: > >>Attached is a patch against svn ToT incorporating this fix from ITK: >><http://review.source.kitware.com/#/c/6025/> >> >>It fixes many warnings from clang. > >Recent changes to svn meant that this patch no longer applied. New >patch attached. |
From: Peter V. <pet...@ya...> - 2012-10-09 15:36:36
|
Personally, I'm in favour of promoting vnl_math to become a namespace instead of a class. However, up to now, the programming style conventions of vxl prohibit the use of namespaces in vxl core. So we should first decide to remove this rule before going on with the patch! -- Peter. |
From: Sean M. <se...@ro...> - 2012-10-12 15:36:30
|
On Tue, 9 Oct 2012 16:36:24 +0100, Peter Vanroose said: >Personally, I'm in favour of promoting vnl_math to become a namespace >instead of a class. > >However, up to now, the programming style conventions of vxl prohibit >the use of namespaces in vxl core. So we should first decide to remove >this rule before going on with the patch! I see. In this case, it's not just a matter of style, but of correctness. The current code is not valid C++03. Granted, it is accepted by many compilers, but still. Perhaps there is another solution that makes it both correct and fits the current style guides? I dunno... Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Amitha P. <ami...@us...> - 2012-10-15 01:25:37
|
Sean, I'm not sure what the core issue is. It appears that in both C++98 and C++03, non-integral static constant initialization cannot be done in the header file because of the one definition rule (ODR). vxl/vnl uses the compile-check to see if the compiler allows this for floating point types as an extension. Is it the case that clang supports this extension, but creates a lot of warnings about it? If so, the solution is likely to simply provide an improvement to the try-compile that detects clang and disables the use of the extension. As to the ITK fix: is it the case that putting these in a namespace somehow works around the ODR for non-integral types? I don't think that's true in C++98; don't know about C++03. A pointer to chapter and verse would help, because if C++98 allows the initialization to happen in the header file, then that's a strong argument for introducing a namespace here. Thanks, Amitha. On 9/28/2012 3:26 PM, Sean McBride wrote: > On Fri, 21 Sep 2012 14:42:39 -0400, Sean McBride said: > >> Attached is a patch against svn ToT incorporating this fix from ITK: >> <http://review.source.kitware.com/#/c/6025/> >> >> It fixes many warnings from clang. > Recent changes to svn meant that this patch no longer applied. New patch attached. > > Cheers, > > > > ------------------------------------------------------------------------------ > Got visibility? > Most devs has no idea what their production app looks like. > Find out how fast your code is with AppDynamics Lite. > http://ad.doubleclick.net/clk;262219671;13503038;y? > http://info.appdynamics.com/FreeJavaPerformanceDownload.html > > > _______________________________________________ > Vxl-maintainers mailing list > Vxl...@li... > https://lists.sourceforge.net/lists/listinfo/vxl-maintainers |
From: Brad K. <bra...@ki...> - 2012-10-15 12:23:04
|
On 10/14/2012 09:25 PM, Amitha Perera wrote: > As to the ITK fix: is it the case that putting these in a namespace > somehow works around the ODR for non-integral types? Yes. There was never an ODR problem. The "One Definition Rule" says that the definition of a template must be the same in all translation units where it appears in one program. This is true. The problem was in making the symbol appear in all translation units without duplicate symbol linker errors. > A pointer to chapter and verse would help The patch includes a comment with such a reference: + /* Strictly speaking, the static declaration of the constant + * variables is redundant with the implicit behavior in C++ + * of objects declared const as defined at: + * "C++98 7.1.1/6: ...Objects declared const and + * not explicitly declared extern have internal + * linkage." + * + * Explicit use of the static key word is used to make the + * code easier to understand. Since they get internal linkage there is no problem with duplicate symbols. The definitions are all the same so there is no ODR trouble. >, because if C++98 allows the initialization to happen in the header file, > then that's a strong argument for introducing a namespace here. Yes. FYI, CMake builds on all kinds of ancient compilers and it uses namespaces. It is not a portability limitation anymore. -Brad |
From: Sean M. <se...@ro...> - 2012-10-15 14:23:26
|
On Sun, 14 Oct 2012 21:25:25 -0400, Amitha Perera said: >I'm not sure what the core issue is. It appears that in both C++98 and >C++03, non-integral static constant initialization cannot be done in the >header file because of the one definition rule (ODR). vxl/vnl uses the >compile-check to see if the compiler allows this for floating point >types as an extension. What Brad said... :) >Is it the case that clang supports this >extension, but creates a lot of warnings about it? Exactly. Thousands and thousand of such warnings... >If so, the solution >is likely to simply provide an improvement to the try-compile that >detects clang and disables the use of the extension. I still think it would be even better to actually make the code C++-conformant, then you don't need any try-compile trickery that needs tweaking every time there's a new compiler or compiler change. Cheers, -- ____________________________________________________________ Sean McBride, B. Eng se...@ro... Rogue Research www.rogue-research.com Mac Software Developer Montréal, Québec, Canada |
From: Amitha P. <ami...@us...> - 2012-10-19 15:10:09
|
Yes, I guess the ODR issues were created by using static members, and the namespace solution "fixes" that. I support applying this patch. Namespaces are well-enough supported that this will not create any real issues. Amitha. On 10/15/2012 10:23 AM, Sean McBride wrote: > On Sun, 14 Oct 2012 21:25:25 -0400, Amitha Perera said: > >> I'm not sure what the core issue is. It appears that in both C++98 and >> C++03, non-integral static constant initialization cannot be done in the >> header file because of the one definition rule (ODR). vxl/vnl uses the >> compile-check to see if the compiler allows this for floating point >> types as an extension. > What Brad said... :) > >> Is it the case that clang supports this >> extension, but creates a lot of warnings about it? > Exactly. Thousands and thousand of such warnings... > >> If so, the solution >> is likely to simply provide an improvement to the try-compile that >> detects clang and disables the use of the extension. > I still think it would be even better to actually make the code C++-conformant, then you don't need any try-compile trickery that needs tweaking every time there's a new compiler or compiler change. > > Cheers, > |
From: Peter V. <pet...@ya...> - 2012-10-22 20:18:47
|
Is there general agreement on this change (and implicitly on now allowing namespaces in vxl core)? If so, I'll apply the change from "class vnl_math" to "namespace vnl_math" by the end of this week. -- Peter. -- On 2012-10-19 Amitha Perera <ami...@us...> wrote: > [...] I support applying this patch. Namespaces are well-enough > supported that this will not create any real issues. -- |
From: Gehua Y. <yan...@gm...> - 2012-10-23 00:59:41
|
I did not follow this issue closely, but I feel like namespaces are so much better supported now compared to early 2000's that I am comfortable allowing namespaces in vxl core. (Just my two cents) Regards, Gehua On Mon, Oct 22, 2012 at 4:18 PM, Peter Vanroose <pet...@ya...>wrote: > Is there general agreement on this change (and implicitly on now allowing > namespaces in vxl core)? > > If so, I'll apply the change from "class vnl_math" to "namespace vnl_math" > by the end of this week. > > -- Peter. > > > -- On 2012-10-19 Amitha Perera <ami...@us...> wrote: > > [...] I support applying this patch. Namespaces are well-enough > > supported that this will not create any real issues. > > > > > > > > > > > > > > > > > > > > > > > > -- > > > ------------------------------------------------------------------------------ > Everyone hates slow websites. So do we. > Make your web apps faster with AppDynamics > Download AppDynamics Lite for free today: > http://p.sf.net/sfu/appdyn_sfd2d_oct > _______________________________________________ > Vxl-maintainers mailing list > Vxl...@li... > https://lists.sourceforge.net/lists/listinfo/vxl-maintainers > |
From: Antonio G. C. <A.G...@de...> - 2012-10-23 07:30:14
|
It's a good idea. In fact, I think we should include new features as soon as the compilers are sufficiently stable. Antonio. El 22/10/12 22:18, Peter Vanroose escribió: > Is there general agreement on this change (and implicitly on now allowing namespaces in vxl core)? > > If so, I'll apply the change from "class vnl_math" to "namespace vnl_math" by the end of this week. > > -- Peter. > > > -- On 2012-10-19 Amitha Perera <ami...@us...> wrote: >> [...] I support applying this patch. Namespaces are well-enough >> supported that this will not create any real issues. > > > > > > > > > > > > > > > > > > > > > > > -- > > ------------------------------------------------------------------------------ > Everyone hates slow websites. So do we. > Make your web apps faster with AppDynamics > Download AppDynamics Lite for free today: > http://p.sf.net/sfu/appdyn_sfd2d_oct > _______________________________________________ > Vxl-maintainers mailing list > Vxl...@li... > https://lists.sourceforge.net/lists/listinfo/vxl-maintainers |