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

[Julien Grall <julien.grall@linaro.org>]

On 03/26/2014 04:42 PM, Jan Beulich wrote:
>>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote:
>> On 03/26/2014 11:57 AM, Jan Beulich wrote:
>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> 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.

-- 
Julien Grall