Page MenuHome

Warning Fixes 11-16-2012
Closed, ResolvedPublicPATCH

Description

A size type was getting negated and cast to int in MEM_CacheLimiter.h. I silenced this warning, but I think that this code could possibly fail in some cases.

Changed an add_definitions to remove_definitions in the OpenNL CMakeLists.txt

Disabled warnings about pragmas in several files where MSVC was complaining about GCC pramas.

On WIN32 systems I undefined stat and fstat and redefine them as _stat and _fstat. It appears that the wrong stat structure is getting passed to _wstat, at least on my 32-bit system.

Added a BLI_MATH_DO_INLINE macro to control inlining of BLI math routines instead of __BLI_MATH_INLINE_H__. This lets the .c files be compiled and actually export symbols. I also think even though the file seems to be designed to allow disabling inlining, that maybe this didn't actually work.

In string.c a bunch of functions were declared with __restrict but the actual function definitions didn't have the qualifier.

A double -> float conversion was fixed in readfile.c

I made it so that node_composite_util.c and node_composite_util.h are excluded from the project files if the legacy compositor is not enabled. They don't export any symbols in that case.

Event Timeline

MEM_CacheLimiter: why not change priority variables from int to size_t then?

OpenNL: I don't think remove_definitions will work, at least not with GCC, DEBUG is a built in define from the compiler.

BLI math inline: I can see the bad reuse of the variable name, but why add the "#ifndef BLI_MATH_DO_INLINE" in the .h and .c files? As far as I can see it's not needed, by including BLI_math which includes BLI_math_inline.h it will get defined. It worked fine when I implemented it, but no compiler needed it.

BLI_fileops: I'd move those defines to BLI_fileops_types.h where it includes <sys/stat.h> and does some similar checks for direntry. I don't fully understand this though, there seem to many different struct types and functions, should be cleaned up properly once.

> MEM_CacheLimiter: why not change priority variables from int to size_t then?
Because it touches a more code than I wanted to mess with and it is a paranoid case. I don't think it can happen on a 32-bit machine and would require you to allocate over 16GB on a 64-bit machine. (But I'd have to check again to be sure.)

> OpenNL: I don't think remove_definitions will work, at least not with GCC, DEBUG is a built in define from the compiler.
Maybe I could do an "if (MSVC) ... " for this case then.

> BLI_math_inline: ...
If we never want to be able to turn off inlining then we should change the .c files to .h so they do not get compiled. If we want to optionally turn it off and on then something like this patch will do it. It is also "fine" as it is, but there is a warning about the .c files containing no exportable symbols and that is what I'm trying to quiet.

> BLI_fileops: ...
If there is a better spot then I'll move it. The problem is that BLI_stat is mismatching the definition of 'stat' and '_wstat' for my 32-bit machine. It actually is kind of confusing. I first made this change to swiss_cheese after a bit of research, but I cannot remember the exact justification now. I should probably write a test case for all the code that uses 'stat' to make sure it works. When I first looked at this nothing actually seemed to use BLI_stat.

> MEM_CacheLimiter
It's indeed an unlikely case, you not only need more than 16GB but also also 2147483648 entries in the cache. Anyway, maybe at a note that says this and remove the XXX, to me that indicates something is really broken.

> OpenNL: I don't think remove_definitions will work, at least not with GCC, DEBUG is a built in define from the compiler.
> Maybe I could do an "if (MSVC) ... " for this case then.
Right, that seems a good solution.

> BLI_math_inline: ...
I don't see how your warning fixes this then though, does it make the files actually export symbols? That shouldn't happen. Better to not build the files then, or find a way to suppress that warning with a compiler flag.

> BLI_fileops
Looking at the code it seems like this change would be ok but if you want to change this better stand by to fix things if it breaks builds. BLI_fileops_types.h is include in BLI_fileops.h anyway, just a minor change I think, to keep stat stuff grouped a bit.

Re: BLI_math_inline, I'll probably go with a warning suppression. I do not want to exclude them from the build because then they won't show up in MSVC project files.

Re: BLI_fileops, well, it is definitely wrong now, unless the two stat structures happen to have the same layout, because the pointer is of the wrong type. It wouldn't even compile if it was C++ because that kind of thing is actually an error in C++



Ok, if you can find a way to suppress the warnings in BLI_math_inline then I guess that covers everything. So you can go ahead and commit that then without further review from me :)

note, quieting unknown pragma's shouldn't be needed, I've added #ifdef __GNUC__ to the diagnostics.

I resolved it because I wasn't sure if clang understood the gcc pramas or not or what would be defined for clang vs. gcc, so I figured I'd just fix it for MSVC

oops, that wasn't very clear, I meant: I resolved it [the pragma warning] *that way* because...

I'm going to commit everything but the changes to stat. I need to sort out the differences between MSVC and MinGW (Hint, I don't think there actually should be any differences).

Right now even if the wrong type is being passed to wstat it doesn't matter, and even if it did it would only show the wrong modified/create/access time. But the conditional compilation is overly complicated. I'll sort it out but I'll have to set up a MinGW compiler before I can test it fully.

Followup on stat fixes here:
http://projects.blender.org/tracker/?func=detail&aid=33288&group_id=9&atid=127

Jason Wilkins (jwilkins) changed the task status from Unknown Status to Resolved.Nov 24 2012, 4:26 PM