Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#96 needless explicit cast?

open
None
5
2012-03-16
2012-03-11
Yoichi NAKAYAMA
No

Recently Fabrice introduced some explicit casts. But I think some of those are not necessary.
For example, in function parse_status_t skip_blank_lines() in httpparser.c:

parse_status_t status;
...
if (status == (parse_status_t)PARSE_OK) {

where PARSE_OK is nothing other than parse_status_t . I think the cast does not have any effect.
Why those cast are needed?

Discussion

  • Hi Yoichi,

    I added those casts on enums because otherwise we got the following warnings/defects on Coverity:

    CID 14979: Implicit integer conversion (MISRA_CAST)MISRA-2004 Rule 10.1 violation: implicitly changing the signedness of an expression. Converting "4", with underlying type "char" (8 bits, signed), to type "unsigned int" (32 bits, unsigned) with different signedness.
    538 if (status == PARSE_OK) {
    539 /* pushback a non-whitespace token */
    540 scanner->cursor -= token.length;
    541 }

    My personal interpretation is that enum should only be used in switch (in this case, there is no error in Coverity). If enum are used in if statement, we need to cast them.

    Best Regards,

    Fabrice

     
  • I also casted a lot of 1 or 2 into (size_t)1 or (size_t)2 as otherwise we got also the following errors:

    CID 15010: Implicit integer conversion (MISRA_CAST)MISRA-2004 Rule 10.1 violation: implicitly changing the signedness of an expression. Converting "2", with underlying type "char" (8 bits, signed), to type "unsigned int" (32 bits, unsigned) with different signedness.
    455 memcpy(file_buf + num_read, "\r\n", 2);

    Fabrice

     
  • Implicit cast a feature of C language, and it is not always a problem.
    If it causes a problem on behavior of the program or a problem on readability of the source code, it should be fixed, of course.
    But I think it is not a good policy to use explicit cast everywhere. Since it imposes meaningless casts on developers, and it can hide real problems.

     
  • I think that casting 2 into (size_t)2 is not a real problem.
    Casting i into (size_t)i can hide a real issue as i can be negative or greater than size_t.
    So, the correct policy should be to check that i is greater than 0 and that is i is lower than the maximum of size_t.
    However, if there was an issue in pupnp, it will be present with or without the cast.

    For enums, I totally agree that casting PARSE_OK into parse_status_t is not an elegant solution.
    An other option would be to remove all enum constants from if statement and use a variable instead as below:

    parse_status_t status2 = PARSE_OK;
    do {
    status = scanner_get_token(scanner, &token, &tok_type);
    } while (status == status2)

    I find some information between enum objects and enum constants and MISRA-C here: http://www.misra-c2.com/forum/viewtopic.php?f=66&t=1091&sid=c31c7b4b1a5ca9c429e9eddc870b2c27.

    An other option is to replace all if statements by a switch.

     
  • I understand that there is no real problem currently, and the tool warns since there is a possibility to cause problem if we modify related code in future.

    I think it is OK to change code to suppress warnings.
    But we would better be careful on: not to introduce real problems, not to impair readability of code significantly.

    Best regards,

     
    • status: open --> closed
     
    • status: closed --> open
     
  • I am sorry I let you take more time.
    On the enum case, I think it is always safe to implicitly cast PARSE_OK (it has int type by C specification)
    to the underlying type of parse_status_t, since underlying type must be selected to ensure it can include
    PARSE_OK in its range.
    Isn't it a analyzer issue?