Re: [Xen-devel] [PATCH 05/34] xen/xsm: flask: Remove unused function avc_sidcmp

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 03/26/2014 05:06 PM, Julien Grall wrote:
> On 03/26/2014 04:42 PM, Jan Beulich wrote:
>>>>> On 26.03.14 at 17:11, <julien.grall@xxxxxxxxxx> wrote:
>>> On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>>>>> On 25.03.14 at 17:55, <julien.grall@xxxxxxxxxx> wrote:
>>>>> Fix compilation with Clang 3.5:
>>>>>
>>>>> avc.c:657:19: error: unused function 'avc_sidcmp' 
>>>>> [-Werror,-Wunused-function]
>>>>> static inline int avc_sidcmp(u32 x, u32 y)
>>>>
>>>> Despite Daniel having acked this already, this seems conceptually wrong
>>>> to me: static inline functions are quite frequently unused (and I assume
>>>> the compiler warns about them only if they're not in a header file), and
>>>> hence the compiler shouldn't be warning about them in the first place.
>>>
>>> This function has not been used seen 2007. I think this is the only
>>> static inline function not defined in headers which is not used.
>>>
>>> Why shouldn't the compiler warn about it? Rather than static inline
>>> function defined in the header, this kind function is dead code. If we
>>> want to keep it we should at least mark them as unused.
>>>
>>> IHMO I don't think we need to keep function that weren't used since ages
>>> and I bet it was a forgotten when the code was reworked a long time ago.
>>
>> Yes, and my comment wasn't about this specific function, but the
>> general pattern: You justified your change with the build breakage,
>> whereas you could have stated what you said just now in your
>> reply. IOW I'm fine with you cleaning up things that were more or
>> less obviously forgotten in an earlier change. But code adjustments
>> just to satisfy an overly picky compiler aren't that nice. After all
>> such functions can serve a purpose (and I think if you looked around
>> you'd find other unused stuff that could be deleted yet - so far - isn't
>> being, as being potentially useful in the future), and the compiler here
>> - from what I can tell - is warning on these simply because they aren't
>> in header files. Which in turn raises the question how the compiler
>> knows what a header file is (remember that we have quite a few
>> instances of .c files including other .c files, and I'd bet the compiler
>> treats these as header files too). Summary: Likely the compiler is
>> issuing this sort of warning inconsistently, and hence it would better
>> not issue the warning at all (or at least provide a way to suppress it).
> 
> Thanks for your comment. I will update the different commit message.
> 
> The new version of clang has amazing feature like guard checking (see
> patch #17)... I was wondering the same thing when I first discovered
> this kind of errors.
> 
> I guess with the '# 1 "/local/.../.h" hints the compiler is able to know
> where does the error come from.
> 
> I will try to find why clang is considering static inline invalid in .c
> context.
> 

Here we go, this has been explicitly added in clang a while ago:

commit 39bd371610af27b073c792c54c6c28133329f6ad
Date:   Tue Sep 10 03:05:56 2013 +0000

    Make -Wunused warning rules more consistent.
    
    This patch does a few different things.
    
    This patch improves unused var diags for const vars: we no longer
    unconditionally suppress diagnostics for const vars, instead only 
suppressing
    the diagnostic when the declaration appears to be useful.
    
    This patch also makes us more consistently use whether a variable/function
    is declared in the main file to suppress diagnostics where appropriate.
    
    Fixes <rdar://problem/14907887>.
    
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190382 
91177308-0d34-0410-b5e6-96231b3b80d8

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel