Bugs, ,bugs, and more bugs: PVS Studio
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/
Tav
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...> 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/Tav
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
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,
This has been reported by the PVS-Studio team as Bug #1613662 [1]. I have added a "to do" list to the summary for anyone who is interested in working on this. Many should be easy fixes.
I have also requested a copy of the full analysis report.
AV
[1] https://bugs.launchpad.net/inkscape/+bug/1613662
On 16 August 2016 at 03:52, Martin Owens <doctormo@...400...> wrote:
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,
Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
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
Le 16/08/2016 à 18:44, mathog a écrit :
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; }
According to Inkscape coding style: https://inkscape.org/en/develop/coding-style/ I suggest you write:
if (blah) do_something;
// or
if (blah) { do_something; }
instead.
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.
Yeah, I noticed it reading the code and attempting to find a better solution (and I felt ashamed of my comment). My personal solution would be to run the two cases without checking and checking both at the end:
if (cond) { a = load(); b = load(); if (a && b) { run(a, b); } }
thus b will try to load even if a failed — we expect it will be an unusual case. In that case, we could also have:
if (!a) return;
just after trying to load a, as all the below statements will be useless in the function. But it lacks symmetry.
The one-time loop is not so bad after all. Note that Java doesn't have any goto instruction, but its break instruction can accept a label.
Le 16/08/2016 à 20:39, Michael Soegtrop a écrit :
I would be careful with the this!=NULL statement for
bool SPLPEItem::performPathEffect
as far as I can tell, this is not a virtual function, and there is no reason why the this pointer of a non virtual function should not be NULL. One can call a non virtual member function on a NULL pointer. Not very nice, but some people put such checks into the code as extra safety check on purpose and some even use tricks like calling a function on NULL pointers on purpose, e.g. for saving a lot of checks when the function is called.
Sure! As, for SomeClass::func being non-virtual, these statements:
SomeClass *some_obj = nullptr; some_obj->func();
are pretty much similar to:
func(some_obj);
whereas if the function is virtual, it'll be something like that:
(some_obj->vtable_ptr_for_func)(some_obj);
and that will cause a segfault when reading vtable_ptr_for_func.
I copied your comments stating your competition with the analyser on the bug report: https://bugs.launchpad.net/inkscape/+bug/1613662 so that PVS-Studio can benefit of their contribution to the Inkscape project — :). -- Sylvain
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
Thanks Tav and pass on a thanks to other Alex.
I really enjoyed reading the article, very interesting.
Unfortunately, this particular static analysis tool is proprietary and bitkeepering inkscape would be a mistake. Although I appreciate the attention the tool brings to out code quality.
(The website's codebase runs over pylint, but nothing so aggressive as this)
Martin Owens
On Mon, 2016-08-15 at 22:18 +0100, Alex Valavanis wrote:
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...> 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/
Tav
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. http://sdm.link/zohodev2dev _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
participants (5)
-
Alex Valavanis -
Martin Owens -
mathog -
Sylvain Chiron -
Tavmjong Bah