Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-13T23:28:23Z

That's just there to appease the static tools. I think it might be unnecessary with the assert() now there, but I added it anyways.

BTW which GCC did you see a problem with? I test with both very old and the almost latest. Please give it a try as I didn't see the problem, and if it breaks send a patch, thanks.

Fixed in git towards 3.911.

Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-14T14:22:34Z

We build with GCC7 with a very strict set of warning checks. Here is the set in case you're curious:
-Wall -Werror -Wextra -Wshadow -Wduplicated-cond -Wlogical-op

When building Verilator I had to explicitly disable some warnings that probably shouldn't have to be disabled:
-Wno-nonnull-compare -Wno-implicit-fallthrough -Wno-shadow

And without this flag the optimizer will remove incorrect null checks in V3AstNodes and cause segfaults:
-fno-delete-null-pointer-checks

For our Clang build I additionally need:
-Wno-undefined-bool-conversion

I've pulled your changes and uploaded a patch that fixes the additional areas that warn. Even though I use -Wno-shadow for builds of Verilator itself, building simulation binaries uses our full set of warnings so any that occur in verilated.h / verilated.cpp will also be caught. There's a shadowing warning that I fixed there and also included in the patch.

Aside: I made this patch with git format-patch. Let me know if it works better than the git diff produced ones I was submitting before.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-15T03:28:08Z

You should "setenv VERILATOR_AUTHOR_SITE 1" in your .cshrc/.bashrc so in the future when you configure warnings will be on by default. This will turn off warnings that are not clean and generally cannot be fixed without a lot of effort, but you're welcome to make patches to beat them down!

I had only rarely been doing stronger than -Wall, but I added -Wextra.

I ended up turning off -Wshadow in the default makefile for generated code, because sometimes the user has global values which are shadowed, and can't be prevented, but it is clean for the moment. There's also a bug in my version of GCC:

if (int foo=...)
else if (int foo=...)

this is not a shadow as they are parallel scopes. Looks like GCC later fixed that but I don't want to make the code less readable with different names.

There was a single fallthrough related to the mess with NORETURN. I fixed that error but it'll probably cause later cppcheck warnings as it seems impossible to keep every tool happy as each deals with these assertion cases slightly differently.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-15T03:30:14Z

Oh also -fno-delete-null-pointer-checks is a known problem discussed in another bug. To fix it I'd need to make 10K+ lines of code less readable which for now I won't do; this isn't needed on Verilated code however, only verilator itself.

Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-15T15:07:12Z

Yeah, I build Verilator within our own build system tree here, which is probably unusual. I'm happy with the current state of things, just felt I should upload any source modifications for others to benefit from as well.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-23T14:15:00Z

In 3.912.