PR: https://github.com/aibasel/downward/pull/171

For completeness: we handled -Wzero-as-null-pointer-constant in issue1112.

BTW, I had a look at the pull request, looks like a nice cleanup.

For -Wshadow, we already have issue1096. If -Wcomment is already enabled that just leaves -Wzero-as-null-pointer-constant. According to https://stackoverflow.com/q/34953361 it is available in GCC since 5.3 and in clang since 5.0. Strange that it isn't listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html then.

I think -Wshadow could be useful, but it would be quite some work to enable it. -Wzero-as-null-pointer-constant also sounds useful, but since it's gone from the docs, we'd first need to check whether all compilers support it and whether it's not already active. -Wcomment is enabled by -Wall.

So what are your thoughts on the other warnings?

Nice, merged.

I'm fine with merging the changes as they are in the pull request now. For the other warnings mentioned in msg11284 we could either create new issues if we want to use them (like issue1096), or ignore them. What do you think?

What is the status/next action for this? I see the status flag is set to "reviewing", but from the discussion it's not clear to me that this is where we are.

Makes sense. I restored the code for printing M&S transitions (by adding a declaration in the header file).

I think the flag is useful.

Regarding the unused code: this is debug code to print something in m&s. As far as I know, we have a lot of such functions in the code that are unused but we decided to keep them around for debugging. I don't really care one way or the other but found it surprising that only one such function is removed.

For -Wshadow, we recently stumbled across this on Windows as well (issue1096). Since the shadowing is not great style, I'd be in favor of getting rid of some of them at least. On Windows we have more fine-grained control about which kind of shadowing leads to a warning and I'd say some are more problematic then others.

In the article you cited there were also two more flags:

-Wzero-as-null-pointer-constant: This sounds useful but I didn't find it in the g++ documentation.

-Wcomment: I explicitly have "-Wno-error=comment" in my .bashrc but I'm not sure what exactly triggers it (especially if the article is correct and -Wcomment is not covered by our flags).

Following the recent addition of the -Wfloat-conversion compiler flag, I wanted to find out which other compiler checks are available. Obtaining all of the ones not covered by "-Wall -Wextra -Wpedantic" is not easy given that new compiler versions add new flags. So I used the list from https://kuczma.dev/articles/gcc-wall/ and checked whether the warnings could be useful for us.

I think the -Wmissing-declarations option is useful for us to detect which functions are missing the "static" keyword. (With this keyword, they are invisible to other compilation units.) I created a PR that enables this flag, adds the missing "static" declarations and also removes an unused (static) function: https://github.com/aibasel/downward/pull/171

The -Wswitch-default flag could be useful for us: we're missing the default case in many switch statements over enums and could abort with a warning in the default case. Hitting the default case can happen if we add a value to the enum option, but forget to adapt the switch statement. If someone wants to tackle this, go ahead :)

The other warnings and why I wouldn't argue for their addition: 

-Wshadow: we shadow variable names in very many places.
-Wdouble-promotion: only useful for old systems.
-Wfloat-equal: we have many comparisons to infinity or 0.
-Wformat-nonliteral -Wformat-security -Wformat-signedness: we don't use printf.

Do you agree that the -Wmissing-declarations flag is useful? Can someone have a look at the pull request, please?