Re: [dpdk-dev] [PATCH v2 1/5] mk/icc: disable treatment of warnings as errors

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/24/2020 7:37 PM, Thomas Monjalon wrote:
> 24/01/2020 17:36, Ferruh Yigit:
>> On 1/23/2020 6:20 PM, Alexander Kozyrev wrote:
>>> Remove -Werror-all flag in ICC configuration file to stop treating ICC
>>> warnings as errors in DPDK due to many false positives. We are using
>>> GCC and Clang as a benchmark for warnings anyway for simplification.
>>>
>>> Suggested-by: Thomas Monjalon <thomas@monjalon.net>
>>> Signed-off-by: Alexander Kozyrev <akozyrev@mellanox.com>
>>> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>>  mk/toolchain/icc/rte.vars.mk | 4 ----
>>>  1 file changed, 4 deletions(-)
>>>
>>> diff --git a/mk/toolchain/icc/rte.vars.mk b/mk/toolchain/icc/rte.vars.mk
>>> index 8aa87aa..1729f3d 100644
>>> --- a/mk/toolchain/icc/rte.vars.mk
>>> +++ b/mk/toolchain/icc/rte.vars.mk
>>> @@ -47,10 +47,6 @@ WERROR_FLAGS += -diag-disable 13368 -diag-disable 15527
>>>  WERROR_FLAGS += -diag-disable 188
>>>  WERROR_FLAGS += -diag-disable 11074 -diag-disable 11076 -Wdeprecated
>>>  
>>> -ifeq ($(RTE_DEVEL_BUILD),y)
>>> -WERROR_FLAGS += -Werror-all
>>> -endif
>>
>> Not sure about removing this globally, as of now the ICC builds fine. If this is
>> for the coming changes in mlx, why not disable warnings in mlx driver only?
> 
> Adding special handling for ICC in the driver means it is kind of supported.
> ICC support is on best-effort basis as a favor to Intel and its CI.
> 
> I don't see any good reason to spend time disabling ICC warnings each time
> we hit a false positive.
> And I would like we stop doing strange things in the code to make ICC happy.
> We do not need to support ICC, or at least we do not need to make ICC build
> warning-free.
> 
> This patch will solve all future annoying issues with ICC in the future.
> 
> Now let me ask the reverse question: why the flag -Werror-all is set for ICC?
> We set this flag for gcc and clang because we use gcc and clang as static
> code analyzers (like coverity) and we want to be sure to catch any issue.
> ICC is not used as code analyzer, this is just an optimization for some
> Intel projects. I won't elaborate on the packaging and licensing of ICC...
> 
> I hope you will be convinced to acknowledge this change.
> 

To support the ICC or not is a valid question, but for me it is better to
support more compilers in case different ones catch an additional issues that is
a benefit for us, although I agree false positives are annoying, which is not
limited to icc.

"-Werror-all" is forced in DEVEL_BUILD, in this cause I was assuming to force
developer to fix the warning to increase the code quality, independent from
compiler.

As of now, all DPDK compiles with icc without warning. But we are allowing the
icc warnings just because the warnings with the change in the mlx driver. And
when we let is once, it is for sure warnings will increase by time.

If we support ICC, I am for keeping this flag and support it properly.