Enable a new compiler warning, fix any issues that ensue #5

Enable a new compiler warning, fix any issues that ensue #5BRL-CAD

Status: ClosedTime to complete: 144 hrs Mentors: Kesha ShahTags: C, C++, compilation, warnings

BRL-CAD's sources are compiled with a number of build flags including numerous warning flags. By default, we treat all warnings as errors and we turn on nearly every warning that the compiler is capable of producing and then we fix them. When you do this systematically and consistently, it can greatly improve code quality. Help us improve BRL-CAD's code quality even more. We already compile with the following gcc/clang warning flags:

-pedantic -Wall -Wextra -Wundef -Wfloat-equal -Wshadow -Winline -Wno-long-long -Wno-variadic-macros -Werror

This task involves enabling any new warning flag that is not already explicitly enabled or enabled by default, and then fixing the warnings that result.

Post a patch for review that enables the new flag, fixes at least one warning, and post a full build log (with "make -k" so we can get an estimate of how many issues need to be fixed).

You can enable the build flag by adding a CHECK_C_FLAG(your_flag_here) line in:

misc/CMake/BRLCAD_CompilerFlags.cmake

Add your line after the "Wall" checks (search for Wall) similar to the numerous other flags being tested. Don't worry about setting it as a C++/CXX flag unless you're feeling adventurous. See the GCC or Clang manual page for a list of warning flags that can be enabled but some unconfirmed possibilities include:

Wcast-qual
Wredundant-decls
Wunreachable-code
Wmissing-declarations
Wmissing-prototypes
Wstrict-prototypes
Wdocumentation (for Doxygen comments)
Wfour-char-constants
Wtraditional
Wbad-function-cast
Wc++-compat
... many many others ... really, see the GCC manpage and try enabling one

How to approach this task? Add your flag, make sure the build halts because something was detected, then run "make -k" to see just how many were detected. Then try to fix just one of them. If you are successful fixing one of them, submit your make -k log and a patch that fixes one and turns on the flag and get started on fixing the rest.

You'll be expected to fix some portion of the warnings reported, but this will be determined on a case-by-case basis depending on how complicated the warnings are and how many there are.

Uploaded Work

File name/URL	File size	Date submitted
http://sourceforge.net/p/brlcad/patches/251/	n/a	January 02 2014 07:12 UTC

Comments

Shardul Chiplunkaron January 1 2014 11:48 UTCTask Claimed

I would like to work on this task.

Mandeep Kaur on January 1 2014 11:57 UTCTask Assigned

This task has been assigned to Shardul Chiplunkar. You have 144 hours to complete this task, good luck!

Shardul Chiplunkaron January 2 2014 07:12 UTCReady for review

The work on this task is ready to be reviewed.

Sean on January 2 2014 07:28 UTCTask Needs More Work

One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.

Sean on January 2 2014 07:29 UTClooking good but partial log

Shardul, this is looking like a good patch, but the build log you submitted is incomplete. If you would, re-submit the log here or regenerate the log. Note that you'll need to redirect stderr to get the warning messages that ensue:

make -k VERBOSE=1 build.log 21

Sean on January 2 2014 07:31 UTCunreachable code

Also, if unreachable code did not detect anything, do not enable it. It very well could already be enabled by default; you'd need to verify with certainty that it wasn't enabled prior. Still better to just focus on one flag.

Shardul Chiplunkaron January 2 2014 07:42 UTCReady for review

The work on this task is ready to be reviewed.

Shardul Chiplunkaron January 2 2014 07:43 UTC

Made the changes, and posted the [quite large] full build log. Is it OK?

Sean on January 2 2014 08:04 UTCunreachable-code

Further confirmation, Wunreachable-code no longer does anything: http://gcc.gnu.org/ml/gcc-help/2011-05/msg00360.html

Sean on January 2 2014 08:10 UTCTask Needs More Work

Sean on January 2 2014 08:14 UTCpatch tested

Patch applied cleanly, but the very first warning/error I encounter after compiling is a declaration mismatch (bu.h and rbinternals.h). That is, you fixed some issues and introduced others with your edits.

This needs to be a patch that can be applied to our sources and still compile cleanly, otherwise, we've made no progress. I realize this may be a fair bit of work for some warning flags, but just keep track of your time and let us know how long the task takes you.

Shardul Chiplunkaron January 2 2014 11:12 UTCReady for review

The work on this task is ready to be reviewed.

Shardul Chiplunkaron January 2 2014 11:13 UTC

It's almost all done except for some complicated pointers in src/libbu/rb_create.c which I can't understand. If you could help me with those, I could fix the warnings generated there.

Daniel Rossberg on January 2 2014 15:32 UTCint (**cfp)(void);

? I haven't tested it. BTW, this are pointers to functions with return type int and no parameters.

Daniel Rossberg on January 2 2014 15:32 UTCTask Needs More Work

Shardul Chiplunkaron January 2 2014 17:19 UTCHow Many?

How many errors (warnings) do I have to fix for this task to get accepted? Is it fine if there are warnings about strict-prototypes, but not about anything else; that is, if I don't exactly break anything?

Shardul Chiplunkaron January 2 2014 18:14 UTCReady for review

The work on this task is ready to be reviewed.

Shardul Chiplunkaron January 2 2014 18:15 UTC

The resulting build log consists of lots of Wstrict-prototype warnings. Should I fix more of them, or is the patch fine?

Sean on January 2 2014 19:59 UTCretry a full build log

This looks like a sufficient quantity of fixes, but the build fails if your new flag is turned off. This means that you have mistakes or something is incomplete. Disable the flag you added in the cmake file and redo your build to see the errors.

Also, make sure you repost a build log with the flag turned ON with "make -k" ... don't forget the -k ;)

Sean on January 2 2014 19:59 UTCTask Needs More Work

Shardul Chiplunkaron January 3 2014 17:29 UTCReady for review

The work on this task is ready to be reviewed.

Sean on January 4 2014 06:13 UTCTask Needs More Work

Sean on January 4 2014 06:41 UTCmany issues

Shardul,

I've just spent several hours (yes, hours) reviewing and fixing your patch and there are simply too many issues to accept the patch without you fixing them. I'm working on applying a modified version of the patch, so you can review my upcoming commits to know what changes need to be made, but I encourage you to attempt the fixes yourself.

Here's a summary of some of the issues I've had to fix:

introducing termio headers in libtermio.h introduced the VMIN shadowing issue. your fix to skip our definition introduces a bug. you need to undefine the system one and if you do one, you should consistently do the other related macros that we define.

you marked all the function args in optical.h as UNUSED... but there's nothing to use, it's just a declaration. even the parameter names can be left off. it's an incorrect use of UNUSED().

if you fix a FIXME, you don't leave the comment .. can you imagine if everyone did that everywhere???

in g_lint.c and remapid.c, you remove the UNUSED(depth) parameter unnecessarily and incorrectly (the function is called with two arguments), which could result in a runtime crash on some platforms. you later cast them to (void (*)(void)) so it's unclear why you even did that.

in rb_create.c, you changed the malloc cast and the sizeof() type, but bizzarely you didn't make them the same parameters.

not an issue, just wanted to say that your use of a (const plane_t *) cast on calls to rt_arb_calc_points is actually surprisingly clever. casts can be evil, but this one is possibly more clever than you realize.

the db_tree_funcleaf() callback parameter casts in pull.c and wdb_obj.c are unnecessary and masked the real bug -- that the callback is taking the wrong type. BUT ... worst of all, you changed the "m" parameter value to a "m" ... which is just very wrong! you introduced two exceptionally obscure bugs that would have been nearly impossible to debug later. don't lose sight of how dangerous changes can be when you're trying to address build warnings/errors, you need to understand you changes and tread carefully when you involve casts that will hide bugs.

your trick in edsol.c to copy the array is no good, now that I see it. for starters, you put a variable declaration in the middle of a scope, which is generally an undesirable practice in C for codes portable to Windows. unclear why you didn't just use your earlier clever cast... which is why I said you probably don't fully understand why it was clever or it would have been obvious to use it.

you should not have modified anything in src/other, especially the removal of const.

Shardul Chiplunkaron January 4 2014 18:28 UTCReady for review

The work on this task is ready to be reviewed.

Shardul Chiplunkaron January 4 2014 18:32 UTC

Saw the errors that occured last time with my patch, and also saw the patches that fixed them -- my earlier patch was quite a mess!

The patch submitted this time is straightforward and should not have any errors. The part I am not sure about is the warnings from the external libs (src/other), and the call to db_tree_funcleaf in db_match.c. Thanks!

Sean on January 4 2014 22:45 UTCTask Closed

Congratulations, this task has been completed successfully.

Sean on January 4 2014 22:51 UTCmuch better

The updated patch looks much better. Nice work.

Your patch was applied in r59278. Your contributor status was graduated from special thanks (for your earlier documentation work) to code contributor status. Thank you for your efforts!