Fix code errors identified by PVS-Studio check

[Egor]

Hello. We've checked the latest version of Inkscape with our PVS-Studio static analyzer, and it found some errors and bugs. You are welcome to read the report: http://www.viva64.com/en/b/0419/

== List of issues (from TOC) ==
* [DONE] Testing a pointer for null after new
* [DONE] Comparing this with zero
* [DONE] Dangerous parameter redefinition
* [DONE] Incorrectly commented-out line
* "One-time" loop
* [DONE] Very odd method
* [DONE] Lost comma
* [DONE] Wrong length in strncmp
* [DONE] Potential division by zero
* Missing else?
* Using a null pointer
* [DONE] Missing semicolon
* [DONE] Unused parameter
* [DONE] Pointer to a nonexistent array
* [DONE] Incorrect object name in a condition
* [DONE] Careless use of numeric constants
* Identical expressions
* Identical operations in if and else blocks

[Alex Valavanis (valavanisalex)]

Thanks Egor,

If you're able to attach a copy of the full analysis report, that would be much appreciated.

[Tavmjong Bah (tavmjong-free)]

I have fixed a number of the bugs (in trunk) already:

* Dangerous parameter redefinition - fixed
* Incorrectly commented-out line - fixed
* Lost comma - fixed
* Wrong length in strncomp - fixed
* Missing semicolon - fixed
* Incorrect object name in a condition - fixed
* Careless use of numeric constants - fixed

[Tavmjong Bah (tavmjong-free)]

* Testing a pointer for null after new.

  Are we allowed to use smart pointers? (It's not listed as allowed in the C++11 features wiki page.) And if so, how does one do it?

  For a quick fix, it looks like one can insert (std::nothrow):

  srcBuf = new (std::nothrow) Byte [srcLen];

[Alex Valavanis (valavanisalex)]

Smart pointers should be fine to use (it was mostly implemented in TR1 anyway). Basically, you just replace:

Thing* my_thing = new Thing();
// do stuff
delete my_thing;

with:

unique_ptr<Thing> my_thing(new Thing());

You can also get rid of the "delete" command, as the Thing will automatically be deleted when it goes out of scope.

If the pointer will be shared by multiple containers, you need to use shared_ptr instead of unique_ptr.

[Sylvain Chiron (frigory)]

Useful comments from the developer list:

Mathog:
> 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:
>
> // not 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.

Here, the analyser says: ‘I'm not sure what the author really wanted this code to do.’ and is just not very clever as it's quite clear in the code.

Michael Soegtrop:
> 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.

Therefore the comment ‘this expression is always false on newer compilers, because 'this' pointer can never be NULL’ by the analyser is probably wrong.

[Tavmjong Bah (tavmjong-free)]

* Identical Expressions:

I believe the following is the correct fix, but perhaps someone more familiar with lib2geom should check:

if(depth > 12 || (Ar.maxExtent() < tol && Ar.maxExtent() < tol)) {

                                          |
                                          V

if(depth > 12 || (Ar.maxExtent() < tol && Br.maxExtent() < tol)) {

[Sylvain Chiron (frigory)]

* Very odd method:

I disagree with the analyzer's solution; it should be:

if (back != nVal) {
    back = nVal;
    ResetPoints();
}

with the surrounding if statement.