Skip to content

Add additional C flags to your build (improving your code) #435

Open
Open
Add additional C flags to your build (improving your code)#435

Description

@melroy89
melroy89
opened on Jan 22, 2022

Try to build with the following C flags and improve your code:

 -Wall -Wextra -Wconversion -Wcast-align -Wstrict-prototypes -Wuninitialized -Wshadow -Wformat=2 -Werror=incompatible-pointer-types

You will get some nice warnings, function declarations isnt' a prototype, unused-parameters and quite a lot of sign-conversion warnings.

Maybe it's worth looking into those warnings! Those are there for a reason... Those warnings can now be spotted and fixed, improving your code quality.

Small snippet from the output (NOT the full output):

[build] ../lib/commonmarker/src/buffer.c:161:39: note: in expansion of macro ‘MIN’
[build]   161 |   int result = memcmp(a->ptr, b->ptr, MIN(a->size, b->size));
[build]       |                                       ^~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_strchr’:
[build] ../lib/commonmarker/src/buffer.c:173:60: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]   173 |       (unsigned char *)memchr(buf->ptr + pos, c, buf->size - pos);
[build]       |                                                  ~~~~~~~~~~^~~~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_drop’:
[build] ../lib/commonmarker/src/buffer.c:211:42: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]   211 |       memmove(buf->ptr, buf->ptr + n, buf->size);
[build]       |                                       ~~~^~~~~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_rtrim’:
[build] ../lib/commonmarker/src/buffer.c:222:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   222 |     if (!cmark_isspace(buf->ptr[buf->size - 1]))
[build]       |                        ~~~~~~~~^~~~~~~~~~~~~~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_trim’:
[build] ../lib/commonmarker/src/buffer.c:237:49: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   237 |   while (i < buf->size && cmark_isspace(buf->ptr[i]))
[build]       |                                         ~~~~~~~~^~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_normalize_whitespace’:
[build] ../lib/commonmarker/src/buffer.c:252:29: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   252 |     if (cmark_isspace(s->ptr[r])) {
[build]       |                       ~~~~~~^~~
[build] ../lib/commonmarker/src/buffer.c: In function ‘cmark_strbuf_unescape’:
[build] ../lib/commonmarker/src/buffer.c:271:54: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   271 |     if (buf->ptr[r] == '\\' && cmark_ispunct(buf->ptr[r + 1]))
[build]       |                                              ~~~~~~~~^~~~~~~
[build] [41/58  58% :: 0.717] Building C object lib/whereami/CMakeFiles/whereami.dir/whereami.c.o
[build] ../lib/whereami/whereami.c: In function ‘wai_getExecutablePath’:
[build] ../lib/whereami/whereami.c:204:29: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]   204 |       memcpy(out, resolved, length);
[build]       |                             ^~~~~~
[build] ../lib/whereami/whereami.c: In function ‘wai_getModulePath’:
[build] ../lib/whereami/whereami.c:329:35: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]   329 |             memcpy(out, resolved, length);
[build]       |                                   ^~~~~~
[build] [42/58  60% :: 0.731] Building C object lib/commonmarker/src/CMakeFiles/LibCommonMarker.dir/references.c.o
[build] In file included from ../lib/commonmarker/src/references.c:1:
[build] ../lib/commonmarker/src/cmark-gfm.h:114:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
[build]   114 | cmark_mem *cmark_get_default_mem_allocator();
[build]       | ^~~~~~~~~
[build] ../lib/commonmarker/src/cmark-gfm.h:120:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
[build]   120 | cmark_mem *cmark_get_arena_mem_allocator();
[build]       | ^~~~~~~~~
[build] In file included from ../lib/commonmarker/src/map.h:4,
[build]                  from ../lib/commonmarker/src/references.h:4,
[build]                  from ../lib/commonmarker/src/parser.h:5,
[build]                  from ../lib/commonmarker/src/references.c:2:
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_ltrim’:
[build] ../lib/commonmarker/src/chunk.h:32:41: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    32 |   while (c->len && cmark_isspace(c->data[0])) {
[build]       |                                  ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_rtrim’:
[build] ../lib/commonmarker/src/chunk.h:42:31: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    42 |     if (!cmark_isspace(c->data[c->len - 1]))
[build]       |                        ~~~~~~~^~~~~~~~~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_strchr’:
[build] ../lib/commonmarker/src/chunk.h:57:61: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]    57 |       (unsigned char *)memchr(ch->data + offset, c, ch->len - offset);
[build]       |                                                     ~~~~~~~~^~~~~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_to_cstr’:
[build] ../lib/commonmarker/src/chunk.h:68:45: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    68 |   str = (unsigned char *)mem->calloc(c->len + 1, 1);
[build]       |                                      ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h:70:27: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]    70 |     memcpy(str, c->data, c->len);
[build]       |                          ~^~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_set_cstr’:
[build] ../lib/commonmarker/src/chunk.h:88:51: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    88 |     c->data = (unsigned char *)mem->calloc(c->len + 1, 1);
[build]       |                                            ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h:90:33: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    90 |     memcpy(c->data, str, c->len + 1);
[build]       |                          ~~~~~~~^~~
[build] [42/58  62% :: 0.770] Linking C static library lib/whereami/libwhereami.a
[build] [42/58  63% :: 0.884] Building C object lib/commonmarker/src/CMakeFiles/LibCommonMarker.dir/blocks.c.o
[build] In file included from ../lib/commonmarker/src/syntax_extension.h:4,
[build]                  from ../lib/commonmarker/src/blocks.c:13:
[build] ../lib/commonmarker/src/cmark-gfm.h:114:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
[build]   114 | cmark_mem *cmark_get_default_mem_allocator();
[build]       | ^~~~~~~~~
[build] ../lib/commonmarker/src/cmark-gfm.h:120:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
[build]   120 | cmark_mem *cmark_get_arena_mem_allocator();
[build]       | ^~~~~~~~~
[build] In file included from ../lib/commonmarker/src/map.h:4,
[build]                  from ../lib/commonmarker/src/references.h:4,
[build]                  from ../lib/commonmarker/src/parser.h:5,
[build]                  from ../lib/commonmarker/src/blocks.c:15:
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_ltrim’:
[build] ../lib/commonmarker/src/chunk.h:32:41: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    32 |   while (c->len && cmark_isspace(c->data[0])) {
[build]       |                                  ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_rtrim’:
[build] ../lib/commonmarker/src/chunk.h:42:31: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    42 |     if (!cmark_isspace(c->data[c->len - 1]))
[build]       |                        ~~~~~~~^~~~~~~~~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_strchr’:
[build] ../lib/commonmarker/src/chunk.h:57:61: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]    57 |       (unsigned char *)memchr(ch->data + offset, c, ch->len - offset);
[build]       |                                                     ~~~~~~~~^~~~~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_to_cstr’:
[build] ../lib/commonmarker/src/chunk.h:68:45: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    68 |   str = (unsigned char *)mem->calloc(c->len + 1, 1);
[build]       |                                      ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h:70:27: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘bufsize_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
[build]    70 |     memcpy(str, c->data, c->len);
[build]       |                          ~^~~~~
[build] ../lib/commonmarker/src/chunk.h: In function ‘cmark_chunk_set_cstr’:
[build] ../lib/commonmarker/src/chunk.h:88:51: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    88 |     c->data = (unsigned char *)mem->calloc(c->len + 1, 1);
[build]       |                                            ~~~~~~~^~~
[build] ../lib/commonmarker/src/chunk.h:90:33: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
[build]    90 |     memcpy(c->data, str, c->len + 1);
[build]       |                          ~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c: In function ‘S_set_last_line_blank’:
[build] ../lib/commonmarker/src/blocks.c:51:20: warning: conversion from ‘int’ to ‘uint16_t’ {aka ‘short unsigned int’} may change value [-Wconversion]
[build]    51 |     node->flags &= ~CMARK_NODE__LAST_LINE_BLANK;
[build]       |                    ^
[build] ../lib/commonmarker/src/blocks.c: In function ‘remove_trailing_blank_lines’:
[build] ../lib/commonmarker/src/blocks.c:221:54: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   221 |     if (c != ' ' && c != '\t' && !S_is_line_end_char(c))
[build]       |                                                      ^
[build] ../lib/commonmarker/src/blocks.c:233:29: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   233 |     if (!S_is_line_end_char(c))
[build]       |                             ^
[build] ../lib/commonmarker/src/blocks.c: In function ‘finalize’:
[build] ../lib/commonmarker/src/blocks.c:284:15: warning: conversion from ‘int’ to ‘uint16_t’ {aka ‘short unsigned int’} may change value [-Wconversion]
[build]   284 |   b->flags &= ~CMARK_NODE__OPEN;
[build]       |               ^
[build] ../lib/commonmarker/src/blocks.c:324:49: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   324 |         if (S_is_line_end_char(node_content->ptr[pos]))
[build]       |                                ~~~~~~~~~~~~~~~~~^~~~~
[build] ../lib/commonmarker/src/blocks.c: In function ‘parse_list_marker’:
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:555:24: note: in expansion of macro ‘peek_at’
[build]   555 |     if (!cmark_isspace(peek_at(input, pos))) {
[build]       |                        ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:562:32: note: in expansion of macro ‘peek_at’
[build]   562 |       while (S_is_space_or_tab(peek_at(input, i))) {
[build]       |                                ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c:577:28: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   577 |   } else if (cmark_isdigit(c)) {
[build]       |                            ^
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:588:42: note: in expansion of macro ‘peek_at’
[build]   588 |     } while (digits < 9 && cmark_isdigit(peek_at(input, pos)));
[build]       |                                          ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:596:26: note: in expansion of macro ‘peek_at’
[build]   596 |       if (!cmark_isspace(peek_at(input, pos))) {
[build]       |                          ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:602:34: note: in expansion of macro ‘peek_at’
[build]   602 |         while (S_is_space_or_tab(peek_at(input, i))) {
[build]       |                                  ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c:33:32: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]    33 | #define peek_at(i, n) (i)->data[n]
[build]       |                       ~~~~~~~~~^~~
[build] ../lib/commonmarker/src/blocks.c:605:32: note: in expansion of macro ‘peek_at’
[build]   605 |         if (S_is_line_end_char(peek_at(input, i))) {
[build]       |                                ^~~~~~~
[build] ../lib/commonmarker/src/blocks.c: In function ‘S_parser_feed’:
[build] ../lib/commonmarker/src/blocks.c:711:30: warning: conversion to ‘char’ from ‘unsigned char’ may change the sign of the result [-Wsign-conversion]
[build]   711 |       if (S_is_line_end_char(*eol)) {

Activity

  1. nwellnhof commented on Jan 23, 2022

    @nwellnhof
    nwellnhof
    on Jan 23, 2022
    Contributor

    Most of these warnings seem to come from -Wconversion which I wouldn't recommend personally.

  2. melroy89 commented on Jan 23, 2022

    @melroy89
    melroy89
    on Jan 23, 2022
    Author

    Most of these warnings seem to come from -Wconversion which I wouldn't recommend personally.

    There are indeed a lot of warnings coming from sign-conversion. But also dozens of warnings from the other flags.

    Maybe you want to explain why you do not recommend that specific flag?

    Edit: the output I show is just a very small output of the whole warning list. So try to build it yourself, to see the WHOLE build log.

  3. nwellnhof commented on Jan 24, 2022

    @nwellnhof
    nwellnhof
    on Jan 24, 2022
    Contributor

    Maybe you want to explain why you do not recommend that specific flag?

    -Wconversion typically generates hundreds of warnings which are subsequently "fixed" by adding explicit casts. But adding an explicit cast doesn't change the compiler output at all, it only codifies a potential programming error. Even worse, after you added explicit casts, there's no way to easily find the original implicit casts again, for example during an audit. AFAIK, explicit casts also disable UBSan checks like -fsanitize=implicit-conversion which would be absolutely counterproductive.

    You'd really have to carefully audit each and every case -Wconversion warns about and ideally add an assertion if you're not absolutely sure no truncation can happen. This is mostly useless and simply too much work for typical open-source projects. These days, fuzzing with sanitizers will catch most of the real-world issues anyway.

    But also dozens of warnings from the other flags.

    I only get about three or four, depending on the compiler.

    • A few -Wstrict-prototypes warnings. These should be fixed and the warning option should be added to the build.
    • On Apple/clang a false positive from -Wformat-nonliteral. In my experience you only want -Wformat=2 if you also annotate your code with format attributes.

    Regarding the other options you propose:

    -Wall -Wextra -Wuninitialized

    We use these already. -Wuninitialized is enabled by -Wextra.

    -Wcast-align

    Should be a no-op on x86. You really want -Wcast-align=strict. This doesn't seem to produce any warnings now. Might be added as a precaution but can also report false positives.

    -Wshadow

    Could be added as a precaution.

    -Werror=incompatible-pointer-types

    This warning is enabled by default. Why should it be made into an error? In my opinion, -Werror should only be used for CI tests.

    In general, -Wall -Wextra is a good default. If a specific warning isn't part of this set, there's typically a good reason. It's up to you to make a case why a specific warning should be enabled. You can't just take a seemingly random list of warnings and ask us to enable them.

  4. melroy89 commented on Jan 24, 2022

    @melroy89
    melroy89
    on Jan 24, 2022
    Author

    Wauw, this is really helpful thanks for your insides and explanation!

    I saw you already made some changes https://github.com/commonmark/cmark/pull/436/files

  5. melroy89 commented on Jan 25, 2022

    @melroy89
    melroy89
    on Jan 25, 2022
    Author

    In my opinion, -Werror should only be used for CI tests.

    I got conflicting answers on that discussion:

    "You should really compile with -Werror which will prevent your compiler from generating code that segfaults"

    gpakosz/whereami#33 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions