[Bf-committers] bugs?

Lars Krueger lars_e_krueger at gmx.de
Sat Feb 12 09:10:01 CET 2011

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

More information about the Bf-committers mailing list