[pacman-dev] meson: use 'pedantic' compiler warning level

Message ID 20200306004231.433695-1-anatol.pomozov@gmail.com
State New
Headers show
Series
  • [pacman-dev] meson: use 'pedantic' compiler warning level
Related show

Commit Message

Anatol Pomozov March 6, 2020, 12:42 a.m. UTC
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

Comments

Eli Schwartz March 6, 2020, 12:50 a.m. UTC | #1
On 3/5/20 7:42 PM, Anatol Pomozov wrote:
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index 572526b2..fc81fa27 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -7,6 +7,7 @@ project('pacman',
>            'prefix=/usr',
>            'sysconfdir=/etc',
>            'localstatedir=/var',
> +          'warning_level=3',

We can just use meson setup --warnlevel 3, the other settings there are
about making sure the software works as expected.

FWIW, meson.build already adds a bunch of -W flags automatically for
buildtype=debug. This would be a better place to go adding even more.
Allan McRae March 6, 2020, 1:08 a.m. UTC | #2
On 6/3/20 10:50 am, Eli Schwartz wrote:
> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  meson.build | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/meson.build b/meson.build
>> index 572526b2..fc81fa27 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -7,6 +7,7 @@ project('pacman',
>>            'prefix=/usr',
>>            'sysconfdir=/etc',
>>            'localstatedir=/var',
>> +          'warning_level=3',
> 
> We can just use meson setup --warnlevel 3, the other settings there are
> about making sure the software works as expected.
> 
> FWIW, meson.build already adds a bunch of -W flags automatically for
> buildtype=debug. This would be a better place to go adding even more.

Agreed.  I'm considering "buildtype=debug" to be the equivalent of
"--enable-git-version --enable-debug --enable-warningflags" in autotools
land.

A
Allan McRae March 6, 2020, 1:51 a.m. UTC | #3
On 6/3/20 11:08 am, Allan McRae wrote:
> On 6/3/20 10:50 am, Eli Schwartz wrote:
>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>>> ---
>>>  meson.build | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 572526b2..fc81fa27 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -7,6 +7,7 @@ project('pacman',
>>>            'prefix=/usr',
>>>            'sysconfdir=/etc',
>>>            'localstatedir=/var',
>>> +          'warning_level=3',
>>
>> We can just use meson setup --warnlevel 3, the other settings there are
>> about making sure the software works as expected.
>>
>> FWIW, meson.build already adds a bunch of -W flags automatically for
>> buildtype=debug. This would be a better place to go adding even more.
> 
> Agreed.  I'm considering "buildtype=debug" to be the equivalent of
> "--enable-git-version --enable-debug --enable-warningflags" in autotools
> land.
>

Also...  I thought all warning flags from autotools were transferred to
meson.  What specific flag is being missed here?

Allan
Anatol Pomozov March 6, 2020, 2:02 a.m. UTC | #4
Hi

On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
> > Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> > ---
> >  meson.build | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/meson.build b/meson.build
> > index 572526b2..fc81fa27 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -7,6 +7,7 @@ project('pacman',
> >            'prefix=/usr',
> >            'sysconfdir=/etc',
> >            'localstatedir=/var',
> > +          'warning_level=3',
>
> We can just use meson setup --warnlevel 3, the other settings there are
> about making sure the software works as expected.
>
> FWIW, meson.build already adds a bunch of -W flags automatically for
> buildtype=debug. This would be a better place to go adding even more.

I see, so the compiler warnings (including the GNU escape symbols
detection) are enabled at Debug mode only.

What is the reason avoid compile warnings with the Release mode?
'meson setup' uses Release mode by default (unless no cmdline options
override it) and I bet most of the contributors are just going to use
this default. Having warnings enabled with the default setup
eliminates potential WTF moments.
Eli Schwartz March 6, 2020, 2:12 a.m. UTC | #5
On 3/5/20 9:02 PM, Anatol Pomozov wrote:
> Hi
> 
> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>>
>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>>> ---
>>>  meson.build | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 572526b2..fc81fa27 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -7,6 +7,7 @@ project('pacman',
>>>            'prefix=/usr',
>>>            'sysconfdir=/etc',
>>>            'localstatedir=/var',
>>> +          'warning_level=3',
>>
>> We can just use meson setup --warnlevel 3, the other settings there are
>> about making sure the software works as expected.
>>
>> FWIW, meson.build already adds a bunch of -W flags automatically for
>> buildtype=debug. This would be a better place to go adding even more.
> 
> I see, so the compiler warnings (including the GNU escape symbols
> detection) are enabled at Debug mode only.
> 
> What is the reason avoid compile warnings with the Release mode?
> 'meson setup' uses Release mode by default (unless no cmdline options
> override it)

Incorrect, meson setup defaults to --buildtype=plain.

Are you using an alias or wrapper script to run meson? That might add
additional options you've forgotten about.

> and I bet most of the contributors are just going to use
> this default. Having warnings enabled with the default setup
> eliminates potential WTF moments.
>
Eli Schwartz March 6, 2020, 2:14 a.m. UTC | #6
On 3/5/20 9:12 PM, Eli Schwartz wrote:
> On 3/5/20 9:02 PM, Anatol Pomozov wrote:
>> Hi
>>
>> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>>>
>>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
>>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>>>> ---
>>>>  meson.build | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/meson.build b/meson.build
>>>> index 572526b2..fc81fa27 100644
>>>> --- a/meson.build
>>>> +++ b/meson.build
>>>> @@ -7,6 +7,7 @@ project('pacman',
>>>>            'prefix=/usr',
>>>>            'sysconfdir=/etc',
>>>>            'localstatedir=/var',
>>>> +          'warning_level=3',
>>>
>>> We can just use meson setup --warnlevel 3, the other settings there are
>>> about making sure the software works as expected.
>>>
>>> FWIW, meson.build already adds a bunch of -W flags automatically for
>>> buildtype=debug. This would be a better place to go adding even more.
>>
>> I see, so the compiler warnings (including the GNU escape symbols
>> detection) are enabled at Debug mode only.
>>
>> What is the reason avoid compile warnings with the Release mode?
>> 'meson setup' uses Release mode by default (unless no cmdline options
>> override it)
> 
> Incorrect, meson setup defaults to --buildtype=plain.

Err, I mean --buildtype=debug, obviously "plain" would be very much not
"debug"...

> Are you using an alias or wrapper script to run meson? That might add
> additional options you've forgotten about.
> 
>> and I bet most of the contributors are just going to use
>> this default. Having warnings enabled with the default setup
>> eliminates potential WTF moments.
>>
> 
>
Anatol Pomozov March 6, 2020, 4:51 a.m. UTC | #7
Hi

On Thu, Mar 5, 2020 at 6:14 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 3/5/20 9:12 PM, Eli Schwartz wrote:
> > On 3/5/20 9:02 PM, Anatol Pomozov wrote:
> >> Hi
> >>
> >> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> >>>
> >>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
> >>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> >>>> ---
> >>>>  meson.build | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/meson.build b/meson.build
> >>>> index 572526b2..fc81fa27 100644
> >>>> --- a/meson.build
> >>>> +++ b/meson.build
> >>>> @@ -7,6 +7,7 @@ project('pacman',
> >>>>            'prefix=/usr',
> >>>>            'sysconfdir=/etc',
> >>>>            'localstatedir=/var',
> >>>> +          'warning_level=3',
> >>>
> >>> We can just use meson setup --warnlevel 3, the other settings there are
> >>> about making sure the software works as expected.
> >>>
> >>> FWIW, meson.build already adds a bunch of -W flags automatically for
> >>> buildtype=debug. This would be a better place to go adding even more.
> >>
> >> I see, so the compiler warnings (including the GNU escape symbols
> >> detection) are enabled at Debug mode only.
> >>
> >> What is the reason avoid compile warnings with the Release mode?
> >> 'meson setup' uses Release mode by default (unless no cmdline options
> >> override it)
> >
> > Incorrect, meson setup defaults to --buildtype=plain.
>
> Err, I mean --buildtype=debug, obviously "plain" would be very much not
> "debug"...

Indeed you are right, meson enables debug mode by default. Sorry for
the red herring.

Returning back to the original question. Can we have a parity in
compiler warnings between Make and Meson? I see that Make uses
-Wpedantic by default while Meson does not.
Allan McRae March 6, 2020, 5:07 a.m. UTC | #8
On 6/3/20 2:51 pm, Anatol Pomozov wrote:
> Hi
> 
> On Thu, Mar 5, 2020 at 6:14 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>>
>> On 3/5/20 9:12 PM, Eli Schwartz wrote:
>>> On 3/5/20 9:02 PM, Anatol Pomozov wrote:
>>>> Hi
>>>>
>>>> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>>>>>
>>>>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
>>>>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>>>>>> ---
>>>>>>  meson.build | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/meson.build b/meson.build
>>>>>> index 572526b2..fc81fa27 100644
>>>>>> --- a/meson.build
>>>>>> +++ b/meson.build
>>>>>> @@ -7,6 +7,7 @@ project('pacman',
>>>>>>            'prefix=/usr',
>>>>>>            'sysconfdir=/etc',
>>>>>>            'localstatedir=/var',
>>>>>> +          'warning_level=3',
>>>>>
>>>>> We can just use meson setup --warnlevel 3, the other settings there are
>>>>> about making sure the software works as expected.
>>>>>
>>>>> FWIW, meson.build already adds a bunch of -W flags automatically for
>>>>> buildtype=debug. This would be a better place to go adding even more.
>>>>
>>>> I see, so the compiler warnings (including the GNU escape symbols
>>>> detection) are enabled at Debug mode only.
>>>>
>>>> What is the reason avoid compile warnings with the Release mode?
>>>> 'meson setup' uses Release mode by default (unless no cmdline options
>>>> override it)
>>>
>>> Incorrect, meson setup defaults to --buildtype=plain.
>>
>> Err, I mean --buildtype=debug, obviously "plain" would be very much not
>> "debug"...
> 
> Indeed you are right, meson enables debug mode by default. Sorry for
> the red herring.
> 
> Returning back to the original question. Can we have a parity in
> compiler warnings between Make and Meson? I see that Make uses
> -Wpedantic by default while Meson does not.
> 

I will happily accept a patch adding -Wpedantic to meson.

A
Anatol Pomozov March 6, 2020, 11:52 p.m. UTC | #9
Hi

On Thu, Mar 5, 2020 at 9:07 PM Allan McRae <allan@archlinux.org> wrote:
>
> On 6/3/20 2:51 pm, Anatol Pomozov wrote:
> > Hi
> >
> > On Thu, Mar 5, 2020 at 6:14 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> >>
> >> On 3/5/20 9:12 PM, Eli Schwartz wrote:
> >>> On 3/5/20 9:02 PM, Anatol Pomozov wrote:
> >>>> Hi
> >>>>
> >>>> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> >>>>>
> >>>>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
> >>>>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> >>>>>> ---
> >>>>>>  meson.build | 1 +
> >>>>>>  1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> diff --git a/meson.build b/meson.build
> >>>>>> index 572526b2..fc81fa27 100644
> >>>>>> --- a/meson.build
> >>>>>> +++ b/meson.build
> >>>>>> @@ -7,6 +7,7 @@ project('pacman',
> >>>>>>            'prefix=/usr',
> >>>>>>            'sysconfdir=/etc',
> >>>>>>            'localstatedir=/var',
> >>>>>> +          'warning_level=3',
> >>>>>
> >>>>> We can just use meson setup --warnlevel 3, the other settings there are
> >>>>> about making sure the software works as expected.
> >>>>>
> >>>>> FWIW, meson.build already adds a bunch of -W flags automatically for
> >>>>> buildtype=debug. This would be a better place to go adding even more.
> >>>>
> >>>> I see, so the compiler warnings (including the GNU escape symbols
> >>>> detection) are enabled at Debug mode only.
> >>>>
> >>>> What is the reason avoid compile warnings with the Release mode?
> >>>> 'meson setup' uses Release mode by default (unless no cmdline options
> >>>> override it)
> >>>
> >>> Incorrect, meson setup defaults to --buildtype=plain.
> >>
> >> Err, I mean --buildtype=debug, obviously "plain" would be very much not
> >> "debug"...
> >
> > Indeed you are right, meson enables debug mode by default. Sorry for
> > the red herring.
> >
> > Returning back to the original question. Can we have a parity in
> > compiler warnings between Make and Meson? I see that Make uses
> > -Wpedantic by default while Meson does not.
> >
>
> I will happily accept a patch adding -Wpedantic to meson.

Eli, could you please take a look at it?
Anatol Pomozov March 26, 2020, 7:32 p.m. UTC | #10
Hello

In this case I would like to propose to move forward with my original
patch and enable 'pedantic' warning level by default. It will match
the flag with the Makefile based build.

On Fri, Mar 6, 2020 at 3:52 PM Anatol Pomozov <anatol.pomozov@gmail.com> wrote:
>
> Hi
>
> On Thu, Mar 5, 2020 at 9:07 PM Allan McRae <allan@archlinux.org> wrote:
> >
> > On 6/3/20 2:51 pm, Anatol Pomozov wrote:
> > > Hi
> > >
> > > On Thu, Mar 5, 2020 at 6:14 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> > >>
> > >> On 3/5/20 9:12 PM, Eli Schwartz wrote:
> > >>> On 3/5/20 9:02 PM, Anatol Pomozov wrote:
> > >>>> Hi
> > >>>>
> > >>>> On Thu, Mar 5, 2020 at 4:50 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
> > >>>>>
> > >>>>> On 3/5/20 7:42 PM, Anatol Pomozov wrote:
> > >>>>>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> > >>>>>> ---
> > >>>>>>  meson.build | 1 +
> > >>>>>>  1 file changed, 1 insertion(+)
> > >>>>>>
> > >>>>>> diff --git a/meson.build b/meson.build
> > >>>>>> index 572526b2..fc81fa27 100644
> > >>>>>> --- a/meson.build
> > >>>>>> +++ b/meson.build
> > >>>>>> @@ -7,6 +7,7 @@ project('pacman',
> > >>>>>>            'prefix=/usr',
> > >>>>>>            'sysconfdir=/etc',
> > >>>>>>            'localstatedir=/var',
> > >>>>>> +          'warning_level=3',
> > >>>>>
> > >>>>> We can just use meson setup --warnlevel 3, the other settings there are
> > >>>>> about making sure the software works as expected.
> > >>>>>
> > >>>>> FWIW, meson.build already adds a bunch of -W flags automatically for
> > >>>>> buildtype=debug. This would be a better place to go adding even more.
> > >>>>
> > >>>> I see, so the compiler warnings (including the GNU escape symbols
> > >>>> detection) are enabled at Debug mode only.
> > >>>>
> > >>>> What is the reason avoid compile warnings with the Release mode?
> > >>>> 'meson setup' uses Release mode by default (unless no cmdline options
> > >>>> override it)
> > >>>
> > >>> Incorrect, meson setup defaults to --buildtype=plain.
> > >>
> > >> Err, I mean --buildtype=debug, obviously "plain" would be very much not
> > >> "debug"...
> > >
> > > Indeed you are right, meson enables debug mode by default. Sorry for
> > > the red herring.
> > >
> > > Returning back to the original question. Can we have a parity in
> > > compiler warnings between Make and Meson? I see that Make uses
> > > -Wpedantic by default while Meson does not.
> > >
> >
> > I will happily accept a patch adding -Wpedantic to meson.
>
> Eli, could you please take a look at it?
Eli Schwartz March 26, 2020, 8:42 p.m. UTC | #11
On 3/26/20 3:32 PM, Anatol Pomozov wrote:
> Hello
> 
> In this case I would like to propose to move forward with my original
> patch and enable 'pedantic' warning level by default. It will match
> the flag with the Makefile based build.
The original patch did not clarify this was about parity with autotools.
I've looked around a bit and my conclusions are as follows:

- We always defaulted to -pedantic -D_GNU_SOURCE -Wall in autotools,
  plus additional flags if --enable-warningflags

- meson will, with warning_level 3, add -Wall -Wextra -Wpedantic, which
  means this patch would add the heretofore-unseen -Wextra
  (warning_level 1 only adds -Wall, 2 adds -Wall -Wextra, no option is
  available for those who like -Wpedantic but don't want -Wextra)

Since autotools unconditionally added these flags, parity suggests
always adding them in meson too. Do we care about -Wextra? We currently
build with -Wextra without warnings being emitted, anyway.

If -Wextra is fine then we can just take this current patch, but amend
it to add -Wextra to configure.ac near

```
if test "x$debug" = "xyes" ; then
```

Anatol, can you also update the commit message to mention that this is
implementing parity with the default autotools warning level?
Anatol Pomozov March 26, 2020, 9:07 p.m. UTC | #12
Hi

On Thu, Mar 26, 2020 at 1:42 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>
> On 3/26/20 3:32 PM, Anatol Pomozov wrote:
> > Hello
> >
> > In this case I would like to propose to move forward with my original
> > patch and enable 'pedantic' warning level by default. It will match
> > the flag with the Makefile based build.
> The original patch did not clarify this was about parity with autotools.
> I've looked around a bit and my conclusions are as follows:
>
> - We always defaulted to -pedantic -D_GNU_SOURCE -Wall in autotools,
>   plus additional flags if --enable-warningflags
>
> - meson will, with warning_level 3, add -Wall -Wextra -Wpedantic, which
>   means this patch would add the heretofore-unseen -Wextra
>   (warning_level 1 only adds -Wall, 2 adds -Wall -Wextra, no option is
>   available for those who like -Wpedantic but don't want -Wextra)
>
> Since autotools unconditionally added these flags, parity suggests
> always adding them in meson too. Do we care about -Wextra? We currently
> build with -Wextra without warnings being emitted, anyway.
>
> If -Wextra is fine then we can just take this current patch, but amend
> it to add -Wextra to configure.ac near
>
> ```
> if test "x$debug" = "xyes" ; then
> ```
>
> Anatol, can you also update the commit message to mention that this is
> implementing parity with the default autotools warning level?

Sure, will send an updated patch in a minute. PTAL.
Eli Schwartz March 27, 2020, 3:36 a.m. UTC | #13
On 3/26/20 4:42 PM, Eli Schwartz wrote:
> On 3/26/20 3:32 PM, Anatol Pomozov wrote:
>> Hello
>>
>> In this case I would like to propose to move forward with my original
>> patch and enable 'pedantic' warning level by default. It will match
>> the flag with the Makefile based build.
> The original patch did not clarify this was about parity with autotools.
> I've looked around a bit and my conclusions are as follows:
> 
> - We always defaulted to -pedantic -D_GNU_SOURCE -Wall in autotools,
>   plus additional flags if --enable-warningflags
> 
> - meson will, with warning_level 3, add -Wall -Wextra -Wpedantic, which
>   means this patch would add the heretofore-unseen -Wextra
>   (warning_level 1 only adds -Wall, 2 adds -Wall -Wextra, no option is
>   available for those who like -Wpedantic but don't want -Wextra)
> 
> Since autotools unconditionally added these flags, parity suggests
> always adding them in meson too. Do we care about -Wextra? We currently
> build with -Wextra without warnings being emitted, anyway.

Allan confirmed on IRC that we do care, and this patch will not be
accepted because we do not want -Wextra.

> If -Wextra is fine then we can just take this current patch, but amend
> it to add -Wextra to configure.ac near
> 
> ```
> if test "x$debug" = "xyes" ; then
> ```
> 
> Anatol, can you also update the commit message to mention that this is
> implementing parity with the default autotools warning level?
>

Patch

diff --git a/meson.build b/meson.build
index 572526b2..fc81fa27 100644
--- a/meson.build
+++ b/meson.build
@@ -7,6 +7,7 @@  project('pacman',
           'prefix=/usr',
           'sysconfdir=/etc',
           'localstatedir=/var',
+          'warning_level=3',
         ],
         meson_version : '>= 0.51')