Wireshark-dev: Re: [Wireshark-dev] Prevent compiler warnings by using "stop on warnings"/"treat
I agree with Ulf. Warnings should not appear (at least most of them)
when compiling wireshark.
For the case "differ in signedness" case, you can use (guchar*). But as
it was already discussed with Guy, wireshark needs a library to handle
strings accurately. One starting point could simply be wrapping string
functions and not use anymore gchar in wireshark but for example, uint8_t.
Concerning, your second point, "warn and blame last committer", I
already thought that for helping the reviewers - which are a few for
wireshark - there could have a "simple" script which could do some
automated job when someone sends a patch to the list. For example :
- if there are new warnings when compiling, etc
- inspect use of "deprecated" C functions, "deprecated" wireshark
functions, use of g_malloc, ... I already wrote a patch for flawfinder
which inspects use of "dangerous" C functions taking into account only
the set of changes induced by the patch. (available in the last
version). We could extend it easily for wireshark purposes (like
inspecting if the patch is using classic memory allocation functions or
deprecated wireshark functions).
Regards,
Sebastien Tandel
Luis Ontanon wrote:
> the point is what kind of warnings can be cleaned up:
>
> to fix "pointer arguments differ in signedness" for example would be
> a waste of time, as they are caused by guint8* used instaed of gchar*
> on those systems (most) that treat char as an unsigned.
>
> But in order to follow that policy you'll have to fill the code with
> "(void*)" casts (which is ugly) beacuse if you simply work on a system
> that treats char as unsigned you can guess what would happen.
>
> BTW
> a more tolerant policy of no *more* warnings (than those we already
> have) would be implementable by something like:
>
> cp warnings last_warnings
> make 2>&1 >warnings
> diff -u warnings last_warnings | grep '^+' || warn_and_blame_last_commiter
>
>
>
> On 3/20/07, Ulf Lamping <ulf.lamping@xxxxxx> wrote:
>
>> Hi List!
>>
>> In my experience having a compiler warning free code is a good way to
>> prevent very subtle bugs and would also be a good addition to the
>> programs security - and BTW more pleasant to work with ;-)
>>
>> You will often hear the following excuse on this topic: "but you cannot
>> write code which won't produce warnings on all those compilers out
>> there". While there are cases where this is true (which has to be
>> handled individually), it's much more often a sign of lazy / ignorant /
>> unskilled developers IMHO.
>>
>> Unfortunately, we're currently having a clear trend to get more and more
>> compiler warnings instead of less. Years ago, I've taken the work to
>> remove warnings of the Win32 build to about 10, today we're having
>> hundreds again :-(
>>
>> While I would be willing to remove code warnings for Win32, I first
>> thought about how to prevent new warnings to rush in - otherwise any
>> work on this topic would be annoying by seeing new warnings rushing in
>> shortly.
>>
>> In my experience, preventing yourself to add new warnings to the WS is
>> currently difficult because of the multi platform support. Even if you
>> provide code that doesn't produce any warnings on "your" working
>> platform, you won't simply notice any new warnings on other platforms
>> (unless you take a look at the buildbot logs) - and compiling on
>> multiple platforms all by yourself is not on option for most of the
>> developers IMO. In the end this is what the buildbot is for.
>>
>>
>> So here comes the buildbot into the scene. If we would use a compiler
>> option like "stop on warnings" (or "treat warnings as errors" or alike),
>> it would become at least much more obvious if new warnings were added -
>> the buildbot will get "red". This will also make the time when a warning
>> is noticed much nearer to the time the code was added/changed -
>> currently fixing a warning once added is often done much later than it
>> was introduced (making the fix unnecessarily difficult).
>>
>>
>> Simply add such a compiler option to the current code state (with it's
>> hundreds of current warnings) would make the buildbot "red" on all
>> platforms for a very long time - which is probably not a good idea.
>>
>> An incremental way to introduce this could be:
>>
>> a) cleanup the warnings for a specific submodule (e.g. wiretap) and
>> platform (e.g. Win32)
>> b) set the compiler option for this submodule only to get a "barrier"
>> for new warnings to rush in
>> c) continue with a) for next submodule :-)
>>
>> As usual, this is my "Win32 point of view". I'm pretty sure the above is
>> possible to do for the Win32 platform. I'm not sure if it's possible
>> with the automake foo for the different unix/linux platform builds ...
>>
>>
>> In effect, the above is to introduce the new coding policy "write
>> warning free code" and a way to "enforce" this.
>>
>> So what's the opinion about this way to improve the Wireshark code base?
>> Are we willing to produce only warning free code and fixing warnings
>> that appear on the buildbot?
>> While I would take a look on the Win32 warnings, are the unix/linux
>> developers willing to spend some time to remove warnings that don't
>> appear on Win32 (or would this be a "Win32 only" show)?
>>
>> Regards, ULFL
>>
>> _______________________________________________
>> Wireshark-dev mailing list
>> Wireshark-dev@xxxxxxxxxxxxx
>> http://www.wireshark.org/mailman/listinfo/wireshark-dev
>>
>>
>
>
>
