[Bf-committers] bugs?

[Lars Krueger]

-------- Original-Nachricht --------
> Datum: Sat, 12 Feb 2011 02:37:48 +0000
> Von: Campbell Barton <ideasman42 at gmail.com>
> An: bf-blender developers <bf-committers at blender.org>
> Betreff: Re: [Bf-committers] bugs?

> You suggest we should eventually make blender warning free, I'd like
> this too but at the moment there are a enough false positives mixed in
> with Clang's warnings.
> Or, at least warnings which will never happen unless blender runs into
> memory corruptions/un-thread-safe code.
> In practice this isn't so simple.

Completely agree here. Working code is more important than nice looking code.

> Examples:
> Clang's static checker considers that checks on a value which is a
> pointer passed to the argument may not be the same twice: Example, see
> checks on (mface[i].flag & ME_SMOOTH)
> http://www.graphicall.org/ftp/ideasman42/2011-01-07-2/report-dOTWay.html#EndPath
> 
> In this case it can be silenced by assigning a const and checking that
> instead:
>    const int smoothnormal = (mface[i].flag & ME_SMOOTH);
>     if(smoothnormal)...
>     ...
> 	if(smoothnormal)...

I think Clang also has a point here. If you are really sure that no other threads can change mface[i], because you know there is a lock a few stack frames above, then this really is a false-positive warning. I think your suggestion also results in code that looks nicer because you have a talking name for the condition.

> ... you can see its unreasonable to assign const's everywhere just
> because a variable could theoretically be changed by another thread
> and cause a check to be true once and false the next.

That also depends on how much more you know than Clang. I'll give you an example why I think consts are a good idea. This is a reocurring error I frequently make at work. I have to work a lot with pre-allocated arrays in real-time processing applications.

int n = something.nThings;
for( int i=0; i<n; ++n)
  do_it( something.things[i]);

Code compiles without a single warning. See the problem? The loop increments n instead of i and happily runs 2^32 iterations on i=0.

const int n = something.nThings;
for( int i=0; i<n; ++n)
  do_it( something.things[i]);

Compile it and you get an error for incrementing n. Same goes for = instead of ==.

> I didn't mean to bring this up since blender's code has lots of
> potential buffer overruns, every so often I try to reduce these by
> using BLI_snprintf and similar but we still have enough unchecked uses
> of sprintf, strcpy().
> Of course these _should_ be changed, on the other hand there are lots
> of bugs in the tracker and hardly any are to do with buffer overruns
> because over time we fixed reported cases.

Agree here. Don't change it unless you know its broken.

> Since you point this out, I changes the function so the values are
> printed directly then the stdout flushed. r34785.

Saw the final printf after I send my reply.

> This is easier then imposing string lengths.

In this case.

> Using (void)value; isn't that strange, Nokia's QT-Toolkit uses a macro
> for this to silence unused argument warnings.
> #define Q_UNUSED(x) (void)x;
>
> We could #ifdef a macro like this for MSVC is needed.
> Not suggesting we add this but I'd prefer this to removing assignments
> potentially adding confusion later.

For a lot of copy-and-paste code in the UI, this is the more practical approach, I agree.

As closing words, I would like to add a big "Thank you" for doing this task of cleaning up these warnings.
-- 
Dr. Lars Krueger


Empfehlen Sie GMX DSL Ihren Freunden und Bekannten und wir
belohnen Sie mit bis zu 50,- Euro! https://freundschaftswerbung.gmx.de