diff mbox series

[LEDE-DEV,1/2] libubox: Don't warn about sign comparisons.

Message ID 1517554332-15236-1-git-send-email-rosenp@gmail.com
State Rejected
Delegated to: John Crispin
Headers show
Series [LEDE-DEV,1/2] libubox: Don't warn about sign comparisons. | expand

Commit Message

Rosen Penev Feb. 2, 2018, 6:52 a.m. UTC
-Wsign-compare was breaking my builds when i disabled MIPS16 on ramips (mt7621). Since this will probably not get fixed, disable it.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Karl Palsson Feb. 2, 2018, 12:54 p.m. UTC | #1
Rosen Penev <rosenp@gmail.com> wrote:
> -Wsign-compare was breaking my builds when i disabled MIPS16 on
> ramips (mt7621). Since this will probably not get fixed,
> disable it.

Given the fine tuned nature of all your prior commits, why are
you now all of a sudden avoiding actually fixing the bad sign
comparisons? Turning them off wholesale sounds like a terrible
idea!

Cheers,
Karl P


> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  CMakeLists.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 57804cf..0449a59 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -3,7 +3,7 @@ INCLUDE(CheckLibraryExists)
>  INCLUDE(CheckFunctionExists)
>  
>  PROJECT(ubox C)
> -ADD_DEFINITIONS(-Os -Wall -Werror --std=gnu99 -g3
> -Wmissing-declarations) +ADD_DEFINITIONS(-Os -Wall -Werror
> --std=gnu99 -g3 -Wmissing-declarations -Wno-sign-compare)
>  
>  OPTION(BUILD_LUA "build Lua plugin" ON)
>  OPTION(BUILD_EXAMPLES "build examples" ON)
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
Rosen Penev Feb. 2, 2018, 3:14 p.m. UTC | #2
On Fri, Feb 2, 2018 at 4:54 AM, Karl Palsson <karlp@tweak.net.au> wrote:
>
> Rosen Penev <rosenp@gmail.com> wrote:
>> -Wsign-compare was breaking my builds when i disabled MIPS16 on
>> ramips (mt7621). Since this will probably not get fixed,
>> disable it.
>
> Given the fine tuned nature of all your prior commits, why are
> you now all of a sudden avoiding actually fixing the bad sign
> comparisons?

It breaks my build. Previous efforts at fixing have been unsuccessful.

> Turning them off wholesale sounds like a terrible
> idea!

-Wsign-compare is part of -Wextra, which is not enabled by default for
a lot of OpenWrt projects. There is at least  one such project that
disabled this warning specifically. I don't think anyone cares.

>
> Cheers,
> Karl P
>
>
>>
>> Signed-off-by: Rosen Penev <rosenp@gmail.com>
>> ---
>>  CMakeLists.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>> index 57804cf..0449a59 100644
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -3,7 +3,7 @@ INCLUDE(CheckLibraryExists)
>>  INCLUDE(CheckFunctionExists)
>>
>>  PROJECT(ubox C)
>> -ADD_DEFINITIONS(-Os -Wall -Werror --std=gnu99 -g3
>> -Wmissing-declarations) +ADD_DEFINITIONS(-Os -Wall -Werror
>> --std=gnu99 -g3 -Wmissing-declarations -Wno-sign-compare)
>>
>>  OPTION(BUILD_LUA "build Lua plugin" ON)
>>  OPTION(BUILD_EXAMPLES "build examples" ON)
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
Karl Palsson Feb. 2, 2018, 5:38 p.m. UTC | #3
Rosen Penev <rosenp@gmail.com> wrote:
> On Fri, Feb 2, 2018 at 4:54 AM, Karl Palsson
> <karlp@tweak.net.au> wrote:
> >
> > Rosen Penev <rosenp@gmail.com> wrote:
> >> -Wsign-compare was breaking my builds when i disabled MIPS16 on
> >> ramips (mt7621). Since this will probably not get fixed,
> >> disable it.
> >
> > Given the fine tuned nature of all your prior commits, why are
> > you now all of a sudden avoiding actually fixing the bad sign
> > comparisons?
> 
> It breaks my build. Previous efforts at fixing have been
> unsuccessful.

link to rejected patches that fix it? Sounds like it would be
easier to push on a "this doesn't compile" than just "let's turn
off warnings"

Cheers,
Karl Palsson
Rosen Penev Feb. 2, 2018, 6:05 p.m. UTC | #4
On Fri, Feb 2, 2018 at 9:38 AM, Karl Palsson <karlp@tweak.net.au> wrote:
>
> Rosen Penev <rosenp@gmail.com> wrote:
>> On Fri, Feb 2, 2018 at 4:54 AM, Karl Palsson
>> <karlp@tweak.net.au> wrote:
>> >
>> > Rosen Penev <rosenp@gmail.com> wrote:
>> >> -Wsign-compare was breaking my builds when i disabled MIPS16 on
>> >> ramips (mt7621). Since this will probably not get fixed,
>> >> disable it.
>> >
>> > Given the fine tuned nature of all your prior commits, why are
>> > you now all of a sudden avoiding actually fixing the bad sign
>> > comparisons?
>>
>> It breaks my build. Previous efforts at fixing have been
>> unsuccessful.
>
> link to rejected patches that fix it?

https://patchwork.ozlabs.org/patch/741841/

Not rejected but no activity. Nobody cares.

> Sounds like it would be
> easier to push on a "this doesn't compile" than just "let's turn
> off warnings"

Except -Wextra is getting added by something. It's not part of
libubox' CFLAGS. AFAIK (away from home so can't check) the other
-Wextra warning that gets triggered is -Wunused-parameter which is
explicitly ignored by the build system with -Wno-unused-parameter.

>
> Cheers,
> Karl Palsson
diff mbox series

Patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 57804cf..0449a59 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -3,7 +3,7 @@  INCLUDE(CheckLibraryExists)
 INCLUDE(CheckFunctionExists)
 
 PROJECT(ubox C)
-ADD_DEFINITIONS(-Os -Wall -Werror --std=gnu99 -g3 -Wmissing-declarations)
+ADD_DEFINITIONS(-Os -Wall -Werror --std=gnu99 -g3 -Wmissing-declarations -Wno-sign-compare)
 
 OPTION(BUILD_LUA "build Lua plugin" ON)
 OPTION(BUILD_EXAMPLES "build examples" ON)