On Sun, Jun 26, 2011 at 3:44 AM, Campbell Barton <ideasman42@...400...> wrote:
On Sat, Jun 25, 2011 at 9:42 PM, Jon Cruz <jon@...18...> wrote:
On Jun 25, 2011, at 1:38 AM, Campbell Barton wrote:
Since blender already built on MSVC/ICC/GCC getting clang working wasn't so hard but it seems inkscape is GCC only and uses a few features not supported in other compilers, Even so, since clang has gcc compatibility it may not be too much work.
Quite so.
Josh and I were just discussing this the past week.
Our goal is to have clean code. However many might lower the priority of building under various commercial compilers (the most common non-gcc compilers to be considered). There are some legitimate philosophical issues to be considered on the point.
On the other hand I had pointed out LLVM as a viable non-gcc, pro-free compiler that we would need to be compatible with. It certainly now seems that this point has moved from the theoretical to the practical and immediate.
So to summarize, in light of having better code (and hence fewer bugs), clang support is a good thing. Additionally if you would like to lend a hand, Coverity had been running analysis on our codebase and it would be good to coordinate with them on viewing the results and getting issues addressed.
Thanks for the contributions.
Hi John, we also had Coverity running on our codebase, but somehow it didn't end up helping that much (IMHO), found some theoretical bugs as static checkers tend to do :), but whoever managed it ran on old source, line numbers didn't match... eh. Just to say in our case I think it was poorly executed, and my experience with running clang locally has given much more useful results.
Thanks for removing the dodgy function which was giving clang all this trouble.
Now the most common issue is variable length arrays on non plain-old-data-types. AKA VLA's on non-POD's
LLVM's page is down atm so pasting from their docs from google cache.
--- snip Variable-length arrays (in C++ section)
GCC and C99 allow an array's size to be determined at run time. This extension is not permitted in standard C++. However, Clang supports such variable length arrays in very limited circumstances for compatibility with GNU C and C99 programs:
The element type of a variable length array must be a POD ("plain old data") type, which means that it cannot have any user-declared constructors or destructors, any base classes, or any members of non-POD type. All C types are POD types. Variable length arrays cannot be used as the type of a non-type template parameter.
If your code uses variable length arrays in a manner that Clang doesn't support, there are several ways to fix your code:
- replace the variable length array with a fixed-size array if you can
determine a reasonable upper bound at compile time; sometimes this is as simple as changing int size = ...; to const int size = ...; (if the initializer is a compile-time constant);
- use std::vector or some other suitable container type; or
- allocate the array on the heap instead using new Type[] - just
remember to delete[] it. --- snip
Here are the error ouput from "make -k", the single dimension arrays are quite easy to convert into heap allocs, but I wasn't able to do this for multi-dimensional arrays. eg: Geom::Point Vtemp[degree+1][degree+1];
Of course with edits to the code a 1d array could be used also means more changes then you may be willing to make to your code and decreases readability somewhat. Perhaps a vector could be used here?, I'm not familiar enough with C++ to know.
/dsk/src/inkscape/src/libcola/shortest_paths.cpp:84:12: error: variable length array of non-POD element type 'shortest_paths::Node' /dsk/src/inkscape/src/libcola/shortest_paths.cpp:94:12: error: variable length array of non-POD element type 'shortest_paths::Node' /dsk/src/inkscape/src/2geom/basic-intersection.cpp:67:26: error: variable length array of non-POD element type 'Geom::Point' /dsk/src/inkscape/src/2geom/solve-bezier-parametric.cpp:196:32: error: variable length array of non-POD element type 'Geom::Point' /dsk/src/inkscape/src/display/nr-filter-gaussian.cpp:528:20: error: variable length array of non-POD element type 'FIRValue' (aka 'FixedPoint<unsigned int, 16>') /dsk/src/inkscape/src/box3d.cpp:458:25: error: variable length array of non-POD element type 'Geom::Point' /dsk/src/inkscape/src/graphlayout.cpp:159:40: error: redefinition of 'i' with a different type /dsk/src/inkscape/src/graphlayout.cpp:160:13: error: invalid operands to binary expression ('list<SPItem *>::iterator' (aka '_List_iterator<SPItem *>') and 'iterator' (aka '_Rb_tree_iterator<value_type>')) /dsk/src/inkscape/src/graphlayout.cpp:163:23: error: member reference base type 'SPItem *' is not a structure or union
Realize this is not priority, rather something that may be worth supporting long term.
Interested to know what you think on the VLA/POD issue.
- Campbell
Small update, built inkscape with clang and it works - as in opens and a quick test worked ok - drawing and moving selections around. I had to comment some chunks of code though, so obviously that cant be committed.
So my question is: Would you accept patches to replace variable length arrays with heap allocations? (val = new Type[size]), delete[] val I can send in patches for it, there are only 5 or so.
For the single dimension arrays this is straightforward and that leaves us with only ~3 multi-dimensional ones which need to be dealt with.