Well, it doesn't really show bugs and is not so embarrassing either… It just points out that some of us code strangely. It detected where the code could be cleaned and it gathers it on a single web page. This is just cool.

Some pieces of codes are WTF… Missing semicolon, lost comma, those may be bugs. Many other things are just very strange: sections ‘One-time loop’, ‘Very odd method’, ‘Comparing this with zero’, actually it's quite funny. There are not as many sections as I could expect actually. Maybe they didn't show all…

Did someone already plan to work on it? Or could I deal with it? Well, I couldn't manage to compile Inkscape trunk yet; I got some errors with GCC 6.1.1 (I could compile 0.92pre1)… But I don't need to compile to correct such things — :). I suppose these should be corrected in the 0.92 branch.

The page says ‘For this analysis, we used the latest version of Inkscape, 0.92, whose source codes can be downloaded from the GitHub repository’. We could tell them that, as our own analysis of their work — :) —, since the source code is not on GitHub yet, and the code for 0.92 probably won't be.

Sylvain

Le 15/08/2016 à 23:18, Alex Valavanis a écrit :

Thanks! Very useful article... The more static analysis tools we can throw at our code, the better!

AV

On 15 Aug 2016 9:21 p.m., "Tavmjong Bah" <tavmjong@...8... mailto:tavmjong@...8...> wrote:

Hi,

Alexandre Prokoudine has pointed out to me an interesting
(embarrassing?) article about our code base. Have a look at:

        http://www.viva64.com/en/b/0419/
<http://www.viva64.com/en/b/0419/>


Tav

On Tue, 2016-08-16 at 03:46 +0200, Sylvain Chiron wrote:

The page says ‘For this analysis, we used the latest version of Inkscape, 0.92, whose source codes can be downloaded from the GitHub repository’. We could tell them that, as our own analysis of their work — :) —, since the source code is not on GitHub yet, and the code for 0.92 probably won't be.

There are some clones on GitHub, some trial runs and some exports.

I think it weakens our developer trademark to have repositories that /look/ legit. A constant problem with github repositories as you can clone anything and your clone looks just like the blessed real thing.

It was a bad design choice for them to not have projects, some sort of trademark protection (even weak protection) some sort of project icon/identifiable mark and maybe even some more aggressive signing for authenticity sake.

But we have the options we have and none of them are great.

Martin,

On 15-Aug-2016 18:46, Sylvain Chiron wrote:

It just points out that some of us code strangely.

That and that some of the code may have been around for a while. One suspects that some of the tests for NULL that the tool flagged as unnecessary may very well have been needed when they were written many years ago in an earlier version of C++. In the worst case these are redundant checks, not errors.

The one thing that it did flag that I agree with 100% is that this:

if(blah) do_something;

is a syntax that should never be used, because editing errors can lead to run on, just like the tool found when the "do_something", a debug statement in that case, was commented out. This happens often enough that I have encountered this problem at least a dozen times in the last 30 years, in a variety of projects. Always use one of these instead:

if(blah)do_something; //if the conditional and action fit entirely on one line

or

if(blah){ do_something; }

In theory the former is subject to the same sort of editing error leading to run on as the original, in practice I have never seen a case of that.

Ditto for "for" loops without braces.

Some pieces of codes are WTF… Missing semicolon, lost comma, those may be bugs.

Those should all be fixed.

Many other things are just very strange: sections ‘One-time loop’

There are instances where coding logic as a one time loop is convenient because then "break" may be used instead of "goto". In some programming projects a religious dispute results if a goto is employed, and this avoids that conflict. As in:

// no really a loop while(1){ if(condition1)break; //code if(condition2)break; //code //etc. break; }

The object file coming out of the compiler probably differs not at all from the goto variant, since the unconditional entry and the final unconditional break tells it that this isn't really a repeating section of code.

Regards,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

In the latter case, there are often cleaner ways of handling "goto-like" situations...

Since we're a C++ project, we can handle exceptional cases by throwing exceptions, type-dependent cases by checking dynamic_casts etc.

Other options include: * using a function instead of a one-time while-loop, and returning instead of breaking. * setting or unsetting a local flag that controls subsequent flow.

AV

On 16 August 2016 at 17:44, mathog <mathog@...1176...> wrote:

On 15-Aug-2016 18:46, Sylvain Chiron wrote:

...

It just points out that some of us code strangely.

That and that some of the code may have been around for a while. One suspects that some of the tests for NULL that the tool flagged as unnecessary may very well have been needed when they were written many years ago in an earlier version of C++. In the worst case these are redundant checks, not errors.

The one thing that it did flag that I agree with 100% is that this:

if(blah) do_something;

is a syntax that should never be used, because editing errors can lead to run on, just like the tool found when the "do_something", a debug statement in that case, was commented out. This happens often enough that I have encountered this problem at least a dozen times in the last 30 years, in a variety of projects. Always use one of these instead:

if(blah)do_something; //if the conditional and action fit entirely on one line

or

if(blah){ do_something; }

In theory the former is subject to the same sort of editing error leading to run on as the original, in practice I have never seen a case of that.

Ditto for "for" loops without braces.

...

Some pieces of codes are WTF… Missing semicolon, lost comma, those may be bugs.

Those should all be fixed.

...

Many other things are just very strange: sections ‘One-time loop’

There are instances where coding logic as a one time loop is convenient because then "break" may be used instead of "goto". In some programming projects a religious dispute results if a goto is employed, and this avoids that conflict. As in:

// no really a loop while(1){ if(condition1)break; //code if(condition2)break; //code //etc. break; }

The object file coming out of the compiler probably differs not at all from the goto variant, since the unconditional entry and the final unconditional break tells it that this isn't really a repeating section of code.

Regards,

David Mathog mathog@...1176... Manager, Sequence Analysis Facility, Biology Division, Caltech

---

---

Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel