lkml.org 
[lkml]   [2006]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: my handy-dandy, "coding style" script
    On Tue, 19 Dec 2006, Jan Engelhardt wrote:

    > (1) Catch casts where they are usually not necessary, because the value
    > would be anyhow discarded
    >
    > (void)strcpy(b, a);
    >
    > (Does not apply to e.g. cmpxchg -- that's why I mentioned (b)!)
    >

    Actually, there's a number of people out there who would like
    (void)strcpy(b, a) or even (void)printf(...) to be required, but the gcc
    manual addresses that point directly:

    10.10 Certain Changes We Don't Want to Make

    - Warning when a non-void function value is ignored

    C contains many standard functions that return a value that
    most programs choose to ignore. One obvious example is printf.
    Warning about this practice only leads the defensive programmer
    to clutter programs with dozens of casts to void. Such casts
    are required so frequently that they become visual noise.
    Writing these casts becomes so automatic that they no longer
    convey useful information about the intentions of the
    programmer. For functions where the return value should never
    be ignored, use the warn_unused_result function attribute.

    So there _is_ a way to manipulate the warning behavior of gcc for this
    point.

    > (2) Catch casts which do not change the type of an expression, e.g.
    >
    > void *x = (void *)kmalloc(...)
    >

    And do what with it? It's a no-op. And when there's tokens such as this
    in source code that become no-ops on compile, it serves as an annotation
    to the code reader that kmalloc returns void*.

    > (3) Catch casts which do not change the outcome, because from-to-void* is
    > warningless
    >
    > struct foo *bar = (struct foo *)kmalloc(...)
    >

    So you would favor some sort of gcc warning such as "unnecessary cast"
    when this is compiled? If so, then there's a _tons_ of other warnings you
    could generate when something exists in source code that does nothing.
    Should we warn on every lone semi-colon that appears or every empty {}
    block?

    int var = (int)(int)(int)13;

    is compiled _exactly_ the same as:

    int var = 13;

    > (4) extension and truncation, implicit conversions - unneessary casts
    >
    > func_taking_u32( (u32)some_u16 );
    > func_taking_u16( (u16)some_u32 );
    >

    Again, these would be no-ops so the casts here actually don't do anything
    and serve as _annotation_ for the code reader since functions aren't
    typically named func_taking_u16. The compiler knows that the function
    takes a u16 and can truncate the actual, but it serves to remind the
    programmer that this value can be truncated later.

    truncate(char ret)
    {
    return (int)ret;
    }

    main()
    {
    int var = 256;
    int ret = test(var);
    return ret;
    }

    This compiles without warning. So if there _was_ an "annotation" such as

    int ret = test((char)var);

    then the programmer is going to catch the bug much easier especially when
    he doesn't check the API of the interface he's using first.

    > (5) Slightly harder one: Where the evaluation process changes, but the
    > outcome is the same
    >
    > #define V_FL_BASE_HI(x) ((x) << 8)
    > #define V_FL_INDEX_LO(x) (x)
    > #define M_FL_INDEX_LO 0xFF
    >
    > static void t3_write_reg(struct adapter *, u32, u32);
    >
    > int t3_sge_init_flcntxt(struct adapter *adapter, unsigned int id,
    > int gts_enable, u64 base_addr, unsigned int size,
    > unsigned int bsize, unsigned int cong_thres, int gen,
    > unsigned int cidx)
    > {
    > t3_write_reg(adapter, A_SG_CONTEXT_DATA1,
    > V_FL_BASE_HI((u32) base_addr) |
    > V_FL_INDEX_LO(cidx & M_FL_INDEX_LO));
    > }
    >
    > (Note that this is not exactly code found in the cxgb3 driver, but
    > tweaked for this example)
    >
    > As far as I can see, even if base_addr was not truncated to u32, the end
    > result would be, when the u64 value is passed to t3_write_reg.
    >

    You would get the same result regardless of whether the cast was there or
    not. But, again, the u32 "cast" appears only as annotation so the
    programmer is aware that the upper 32-bits of base_addr are going to be
    discarded.

    This is precisely why these lines emit the exact same assembly:

    auto int var = (int)(char)256;
    auto int var = (char)256;

    Now, if you wrote:

    auto char var = 256;

    Then this would emit a warning because the programmer did not explictly
    recognize 256 as overflow for a char.

    > __attribute__((used)) would be more appropriate, I think.
    >

    As far as I know, you can't use this on automatic variables and
    -Wuninitialized will still emit the warning message. It's great for the
    static case.

    David
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-12-20 00:13    [W:3.068 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site