GCC 8 introduced `-Wsizeof-pointer-div` which is enabled by `-Wall` and warns for sizeof divisions that look like they would compute the size of a static array but are called on something on which this doesn't work (e.g. a pointer as LHS). This is quite reasonable and useful, but it fails to detect the case where the computation is guarded against being called on problematic values, like our HL_N_ENTRIES() macro that accepts NULLs but won't use the sizeof computation result then.
Work around this by implementing HL_N_ENTRIES() macro in a way that cannot trigger such a warning yet yield the same result.
Example warning: ``` ../../src/highlighting.c: In function ‘highlighting_init_styles’: /usr/include/glib-2.0/glib/gmacros.h:351:42: warning: division ‘sizeof (HLKeyword * {aka struct <anonymous> *}) / sizeof (HLKeyword {aka struct <anonymous>})’ does not compute the number of array elements [-Wsizeof-pointer-div] #define G_N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0])) ^ ../../src/highlightingmappings.h:74:48: note: in expansion of macro ‘G_N_ELEMENTS’ #define HL_N_ENTRIES(array) ((array != NULL) ? G_N_ELEMENTS(array) : 0) ^~~~~~~~~~~~ ../../src/highlighting.c:966:5: note: in expansion of macro ‘HL_N_ENTRIES’ HL_N_ENTRIES(highlighting_keywords_##LANG_NAME)); \ ^~~~~~~~~~~~ ../../src/highlighting.c:1011:3: note: in expansion of macro ‘init_styleset_case’ init_styleset_case(CONF); ^~~~~~~~~~~~~~~~~~ ```
--- Another solution could be reporting a bug to GCC for it to detect our use case and avoid the warning then, but that might or might not be much meaningful, and won't fix the issue on currently affected GCC versions anyway. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2398
-- Commit Summary --
* Rewrite HL_N_ENTRIES macro to avoid a GCC8 false positive warning
-- File Changes --
M src/highlightingmappings.h (12)
-- Patch Links --
https://github.com/geany/geany/pull/2398.patch https://github.com/geany/geany/pull/2398.diff
codebrainz approved this pull request.
Kind of gross, but at least the comment explains it well.
I can think of a couple alternatives, but none which require so few changes (ex. explicit size variables for each array, sentinel-terminated arrays, etc).
@codebrainz its a MACRO, of course its gross :)
@b4n for the places this is used, is there a (compile time testable) value of array element you could use as a non-entry so its always arrays and so doesn't mix arrays and pointers, then the GCC warning will catch if anyone does call it with a pointer?
@b4n for the places this is used, is there a (compile time testable) value of array element you could use as a non-entry so its always arrays and so doesn't mix arrays and pointers, then the GCC warning will catch if anyone does call it with a pointer?
None that I can think of; the non-array values are `EMPTY_KEYWORDS` and `EMPTY_PROPERTIES`. If ISO C allowed it, the simple fix would be to declare those as empty arrays, and it would naturally work out of the box. But without this, the best other solution I can think of is this: ```diff diff --git a/src/highlighting.c b/src/highlighting.c index 938e5432c..82013aa7c 100644 --- a/src/highlighting.c +++ b/src/highlighting.c @@ -961,9 +961,9 @@ static guint get_lexer_filetype(GeanyFiletype *ft) case (GEANY_FILETYPES_##LANG_NAME): \ styleset_init_from_mapping(filetype_idx, config, configh, \ highlighting_styles_##LANG_NAME, \ - HL_N_ENTRIES(highlighting_styles_##LANG_NAME), \ + HL_N_STYLES(highlighting_styles_##LANG_NAME), \ highlighting_keywords_##LANG_NAME, \ - HL_N_ENTRIES(highlighting_keywords_##LANG_NAME)); \ + HL_N_KEYWORDS(highlighting_keywords_##LANG_NAME)); \ break
/* Called by filetypes_load_config(). */ @@ -1068,11 +1068,11 @@ void highlighting_init_styles(guint filetype_idx, GKeyFile *config, GKeyFile *co case (GEANY_FILETYPES_##LANG_NAME): \ styleset_from_mapping(sci, ft->id, highlighting_lexer_##LANG_NAME, \ highlighting_styles_##LANG_NAME, \ - HL_N_ENTRIES(highlighting_styles_##LANG_NAME), \ + HL_N_STYLES(highlighting_styles_##LANG_NAME), \ highlighting_keywords_##LANG_NAME, \ - HL_N_ENTRIES(highlighting_keywords_##LANG_NAME), \ + HL_N_KEYWORDS(highlighting_keywords_##LANG_NAME), \ highlighting_properties_##LANG_NAME, \ - HL_N_ENTRIES(highlighting_properties_##LANG_NAME)); \ + HL_N_PROPERTIES(highlighting_properties_##LANG_NAME)); \ break
/** Sets up highlighting and other visual settings. diff --git a/src/highlightingmappings.h b/src/highlightingmappings.h index d62b7e24b..36fe2d260 100644 --- a/src/highlightingmappings.h +++ b/src/highlightingmappings.h @@ -65,11 +65,14 @@ typedef struct } HLProperty;
-#define EMPTY_KEYWORDS ((HLKeyword *) NULL) -#define EMPTY_PROPERTIES ((HLProperty *) NULL) - -/* like G_N_ELEMENTS() but supports @array being NULL (for empty entries) */ -#define HL_N_ENTRIES(array) ((array != NULL) ? G_N_ELEMENTS(array) : 0) +/* ISO C doesn't allow zero-sized arrays, so we special-case those below */ +static const HLKeyword EMPTY_KEYWORDS[] = {{0}}; +static const HLProperty EMPTY_PROPERTIES[] = {{0}}; + +/* like G_N_ELEMENTS() but supports special values for empty entries */ +#define HL_N_STYLES(ss) G_N_ELEMENTS(ss) +#define HL_N_KEYWORDS(ks) ((ks) == EMPTY_KEYWORDS ? 0 : G_N_ELEMENTS(ks)) +#define HL_N_PROPERTIES(ps) ((ps) == EMPTY_PROPERTIES ? 0 : G_N_ELEMENTS(ps))
/* Abaqus */ ``` but I'm not really convinced it's actually better. I'm not against it, it's not really bad but it's kind of sad it wastes `sizeof(HLKeyword) + sizeof(HLProperty)` (40 bytes here) and introduces 2 new specific macros.
As for getting called with a pointer it's a theoretically good point, but given it's only used on highlightingmappings entries, it should be fairly safe.
I can think of a couple alternatives, but none which require so few changes (ex. explicit size variables for each array, sentinel-terminated arrays, etc).
I really think any solution should minimize the risk of likely problems (well, at least the ones that are likely to happen, e.g. anything that can arise from adding/modifying mappings in higlightingmappings.h), and explicit size variables seem really easy to get wrong (or at the very least tedious), and sentinels are also easily forgotten if not checked by the compiler.
and explicit size variables seem really easy to get wrong
Yeah, in my head I was really thinking that this file could be generated by a script, in which case explicit sizes, when generated by a script, aren't bad.
IMO, if this PR works, just merge it for now and anyone interested in doing it another way can submit a follow-up PR. It's not like it's a big change from the current code.
Merged #2398 into master.
github-comments@lists.geany.org