search

commonmark / cmark

CommonMark parsing and rendering library and program in C
Other
1.62k stars 538 forks source link

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

Open melroy89 opened 2 years ago

melroy89 commented 2 years ago

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)) {
nwellnhof commented 2 years ago

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

melroy89 commented 2 years ago

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.

nwellnhof commented 2 years ago

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.

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.

melroy89 commented 2 years ago

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

melroy89 commented 2 years ago

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"

https://github.com/gpakosz/whereami/issues/33#issuecomment-1019284523