Closed Bug 1324793 Opened 7 years ago Closed 6 years ago

Enable more -Werror warnings (missing-prototypes, redundant-decls, etc)

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: Sylvestre, Unassigned)

Details

Attachments

(1 file)

Bug 1324793 - Enable more -Werror warnings: missing-prototypes & redundant-decls
58 bytes, text/x-review-board-request
Details 
Using the XXX hours of plane to HI, I tried to build Firefox with:
-Werror=missing-prototypes
-Werror=redundant-decls
-Werror=return-type
-Werror=implicit-function-declaration

The two last didn't find anything but the two first found a few things.

I still have a few things to fix in NSS but I would like to get feedback before spending more time on this.
Summary: Enable more -Werror warnings → Enable more -Werror warnings (missing-prototypes, redundant-decls, etc)
QA Contact: sledru
Comment on attachment 8820275 [details]
Bug 1324793 - Enable more -Werror warnings: missing-prototypes & redundant-decls

Chris, what do you think? :)
Attachment #8820275 - Flags: feedback?(cpeterson)
You must have had a power outlet in your seat! :)
Comment on attachment 8820275 [details]
Bug 1324793 - Enable more -Werror warnings: missing-prototypes & redundant-decls

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> -Werror=missing-prototypes
> -Werror=redundant-decls

Generally LGTM. However, we don't want warnings to break the build unless the builder opts in with --enable-warnings-as-errors. Different compiler versions and system header file combinations produce different warnings and we don't want contributors to run into build failures because they updated Xcode or their Linux distro to a version that doesn't match our official build machines.

btw, on clang, -Wmissing-prototypes is not supported and -Wredundant-decls is a no-op, but your check_and_add_gcc_warning() checks handle that.

I'm curious to see the -Wredundant-decls warnings.


> -Werror=return-type

Enabling -Werror=return-type was bug 1236430. It was resolved wontfix because -Wreturn-type is enabled by -Wall and the build machines compile with --enable-warnings-as-errors, so some people may argue that -Werror=return-type is redundant.


> -Werror=implicit-function-declaration

Enabling -Werror=implicit-function-declaration is bug 822978. It has an r+'d patch from 2012 that never landed. -Wimplicit-function-declaration is enabled by -Wall and the build machines compile with --enable-warnings-as-errors, so some people may argue that -Werror=implicit-function-declaration is redundant

I believe there are still -Wimplicit-function-declaration issues with Android's pthread functions.
Attachment #8820275 - Flags: feedback?(cpeterson) → feedback+
Just to summarize, enabling -Wmissing-prototypes and -Wredundant-decls is fine if there aren't a lot of warnings to be fixed or suppressed, but I don't think we should force any of these warnings to be errors with -Werror=*.
Comment on attachment 8820275 [details]
Bug 1324793 - Enable more -Werror warnings: missing-prototypes & redundant-decls

https://reviewboard.mozilla.org/r/99816/#review100412

::: build/moz.configure/warnings.configure:95
(Diff revision 1)
>  
> +# foo
> +check_and_add_gcc_warning('-Werror=missing-prototypes')
> +check_and_add_gcc_warning('-Werror=redundant-decls')
> +check_and_add_gcc_warning('-Werror=return-type')
> +check_and_add_gcc_warning('-Werror=implicit-function-declaration')

These warning checks should be alphabetized.

::: config/external/ffi/moz.build:108
(Diff revision 1)
>          for s in sorted(ffi_srcs)
>      ]
> +    if CONFIG['GNU_CC']:
> +        CFLAGS += [
> +            '-Wno-redundant-decls',
> +            '-Wno-missing-prototypes',

If we think we might fix these warnings, you should suppress them with '-Wno-error=foo' so people still see the warnings.

Also, these warning flags should be alphabetized. Same in other moz.build files below.

::: config/external/nspr/moz.build:33
(Diff revision 1)
>      OS_LIBS += CONFIG['NSPR_LIBS']
> +
> +if CONFIG['GNU_CC']:
> +    CFLAGS += [
> +        '-Wno-missing-prototypes',
> +    ]

Python lists with a single entry should be squeezed into one line like `CFLAGS += ['-Wno-missing-prototypes']`.

::: config/external/nspr/moz.build:35
(Diff revision 1)
> +if CONFIG['GNU_CC']:
> +    CFLAGS += [
> +        '-Wno-missing-prototypes',
> +    ]
> +
> +

Remove trailing whitespace here and in other moz.build files below.

::: dom/canvas/moz.build:199
(Diff revision 1)
>  CXXFLAGS += CONFIG['TK_CFLAGS']
>  
>  LOCAL_INCLUDES += CONFIG['SKIA_INCLUDES']
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-error=shadow']
> +    CXXFLAGS += ['-Wno-error=shadow', '-Wno-redundant-decls']

Python lists with multiple entries should be expanded to multiple lines. Same in other moz.build files below. Also, these warning flags should be alphabetized.

::: dom/media/gmp/GMPAudioDecoderParent.cpp
(Diff revision 1)
>  
>  #ifdef LOG
>  #undef LOG
>  #endif
>  
> -extern LogModule* GetGMPLog();

You should verify whether removing these apparently-redundant GetGMPLog() declarations breaks non-unified builds.

::: dom/media/gmp/GMPService.h:33
(Diff revision 1)
>  
>  namespace mozilla {
>  
>  class GMPCrashHelper;
>  
> -extern LogModule* GetGMPLog();
> +LogModule* GetGMPLog(void);

(void) is unnecessary for declaring functions with no parameters in C++. () means no parameters in C++, though it implies vararg parameters in C.

::: dom/media/mediasource/MediaSource.cpp:16
(Diff revision 1)
>  #include "Benchmark.h"
>  #include "DecoderDoctorDiagnostics.h"
>  #include "MediaContentType.h"
>  #include "MediaResult.h"
>  #include "MediaSourceUtils.h"
> +#include "MediaSourceResource.h"

Header file #includes should be alphabetized.

::: dom/media/mediasource/MediaSourceResource.h:15
(Diff revision 1)
>  #include "MediaResource.h"
>  #include "mozilla/Monitor.h"
>  #include "mozilla/Logging.h"
>  
> -extern mozilla::LogModule* GetMediaSourceLog();
> +mozilla::LogModule* GetMediaSourceLog(void);
> +mozilla::LogModule* GetMediaSourceAPILog(void);

Use () instead of (void) in C+=.
Assignee: nobody → sledru
QA Contact: sledru
Product: Core → Firefox Build System
Assignee: sledru → nobody
Too much work on this one, closing.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: