On Sat, Jun 25, 2011 at 8:24 AM, Jasper van de Gronde <th.v.d.gronde@...528...> wrote:

On 2011-06-25 8:05, Campbell Barton wrote:

...

Hi, I was trying to compile inkscape with clang, and for many reasons this failed, however one looks like it should probably be fixed in the code.

src/ui/widget/registered-widget.h

operator const Gtk::Widget () { return dynamic_castGtk::Widget*(this); }

Should either be:  operator const Gtk::Widget () { return *dynamic_castGtk::Widget*(this); } Or:  operator const Gtk::Widget () { return dynamic_castGtk::Widget(this);

I'm actually a little surprised that it returns a COPY of a Gtk::Widget. It might be more appropriate to return a Gtk::Widget pointer. But in any case I'm a little surprised that this isn't caught using gcc, which makes we wonder whether this is used at all.

BTW, insanely cool that you're trying to use clang to build Inkscape, I'd love to know if and when you succeed.

There are a few issues that will need to be resolved, but will try 1 by 1, when I get time.

Not sure if you guys are familiar with the static checker, This is an older scan of blenders source, we're down to ~600 reports now.

http://www.graphicall.org/ftp/ideasman42/2011-01-07-2/

I found this really useful, many of the reports can be ignored but some of the checks are almost always real bugs - "Result of operation is garbage or undefined" for eg.

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.

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.

On Jun 26, 2011, at 8:44 PM, Campbell Barton wrote:

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.

That sounds good.

However, I'm not sure if I like the solution. If you have code that requires an explicit delete[] to be called, then you have the potential to not call that and then end up leaking.

Of course, I also dislike abuse of autopointers, so there might be something to look at and find the balance for.

The first example that springs to mind is a std::string. That is a handy little class that manages its own memory, and cleans it up when it goes out of scope. So even if one uses multiple early returns (evil, IMHO), or has an exception thrown, etc., cleanup should occur as needed.

Would a std::vector<T> be an appropriate replacement in the areas you're seeing things?

On 27/06/2011, at 3:37 PM, Campbell Barton wrote:

...

Would a std::vector<T> be an appropriate replacement in the areas you're seeing things?

Have to say I don't know C++ well enough to say, heres a simple example of one of the cases with 3 lines of function body:

./src/libcola/shortest_paths.cpp

dijkstra(...) { Node vs[n]; dijkstra_init(vs,es,eweights); dijkstra(s,n,vs,d); }

So not freeing in this case is really not a worry, though maybe some day the function becomes more complicated, which is still an argument against replacing with allocations. Even so there are tools to detect memory leaks, I use valgrind frequently however it does rely on someone running that function to find the leak so errors can still go un-noticed.

Would std::vector<T> be acceptable here?

Since this is code I'm now maintaining (libvpsc, libcola, libavoid), I can answer this. Yes, and in fact this code in the Adaptagrams SVN repository was updated some time ago to use std::vector since someone using it had run into this compiler error.

Updating the libavoid version in Inkscape is on my todo list, and when I do this I'll update libcola and libvpsc as well. I know there were a couple of instances of these dynamically allocated arrays in the Adaptagrams libraries.

Cambpell, are all the instances of this problem in the libcola, libavoid and libvpsc directories? For the ones that are, could you please send me the patch and I'll make sure they are corrected.

Cheers, Michael

------ Dr. Michael Wybrow, Research Fellow Clayton School of Information Technology Monash University Wellington Rd, Clayton, Vic 3800, Australia Phone: +613 9905 2479

Waited for the weekend to do this, heres a patch on inkscapes current source which converts dynamic sized arrays to std::vector's.

http://www.graphicall.org/ftp/ideasman42/clang_compat_r10398.diff

if this can be committed there is only 2 multi dimensional arrays to figure out what to do with, I tried using vectors here too but it didn't work using some examples I found online but rather try solve this in a separate patch.

On Mon, Jun 27, 2011 at 7:58 AM, Jasper van de Gronde <th.v.d.gronde@...528...> wrote:

On 27-06-11 05:44, Campbell Barton wrote:

...

... 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.

Yes, please :) (Although I'd like to echo the earlier comment that it might be better to use std::vector in most cases. Just trying to let you know that this IS interesting.)

On Jun 25, 2011, at 1:24 AM, Jasper van de Gronde wrote:

On 2011-06-25 8:05, Campbell Barton wrote:

...

Hi, I was trying to compile inkscape with clang, and for many reasons this failed, however one looks like it should probably be fixed in the code.

src/ui/widget/registered-widget.h

operator const Gtk::Widget () { return dynamic_castGtk::Widget*(this); }

Should either be: operator const Gtk::Widget () { return *dynamic_castGtk::Widget*(this); } Or: operator const Gtk::Widget () { return dynamic_castGtk::Widget(this);

I'm actually a little surprised that it returns a COPY of a Gtk::Widget. It might be more appropriate to return a Gtk::Widget pointer. But in any case I'm a little surprised that this isn't caught using gcc, which makes we wonder whether this is used at all.

Well, my first reaction when seeing that is to think "aren't we following features of the C++ language itself? Why do we even need a cast?"

operator const Gtk::Widget() { return *this; }

The next thought was on a copy-ness itself. Being const might prevent visible issues, or we might just be lucky. I would think that in general a better solution would be to return a const *reference* to the type, instead of a point or a copy.

Or... do we even need such an operator? Was it perhaps a work-around for some earlier compiler's lack of full template support? gcc 4.2.1 seems quite happy without it.