lkml.org 
[lkml]   [2015]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    Subjectblock: new gcc-5.1 warnings..
    From
    So gcc-5.1 seems to have a few new warnings, most of which seem of
    dubious value, but whatever.

    One of them

    drivers/block/hd.c: In function ‘hd_request’:
    drivers/block/hd.c:630:11: warning: switch condition has boolean value
    [-Wswitch-bool]
    switch (rq_data_dir(req)) {
    ^

    just made me go "what?" since doing a switch on a boolean is perfectly
    fine, and there can be various valid reasons to do so (using "break"
    and fall-through etc can make the structure of the true/false cases
    nicer).

    So the compiler warning is just silly and stupid.

    The warning would make more sense (and still trigger for this kernel
    case) if the case statements then didn't use boolean values.

    So despite the warning in general just being insane, in this case it
    happens to show an oddity of the kernel source code: rq_data_dir()
    returns a boolean, and that actually makes little sense, since it's
    normally compared to READ/WRITE. Which *happen* to be 0/1, and integer
    promotion does the right thing, but if you actually look at what
    READ/WRITE are, it really is 0/1, not false/true.

    This odd boolean came in through commit 5953316dbf90 ("block: make
    rq->cmd_flags be 64-bit") and I think that change really was
    questionable. What happened was that "cmd_flags" got turned into
    "u64", and that commit wants to avoid making rq_data_dir() return a
    u64, because that screws up printk() and friends.

    But I think it might be better off as (I didn't test this):

    #define rq_data_dir(rq) ((int)((rq)->cmd_flags & 1))

    instead, to match the type of READ/WRITE. That would also get rid of
    the (bogus) warning introduced by gcc-5.1.1.

    And maybe somebody could then convince the gcc people that

    switch (boolean) {
    case true:
    ...
    case false:
    }

    is actually perfectly fine. It could still complain about the truly
    odd cases (which the kernel use really arguably is).

    Hmm? Jens?

    Linus


    \
     
     \ /
      Last update: 2015-05-28 00:41    [W:5.015 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site