Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite HL_N_ENTRIES macro to avoid a GCC8 false positive warning #2398

Open
wants to merge 1 commit into
base: master
from

Conversation

@b4n
Copy link
Member

b4n commented Nov 16, 2019

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.

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.
Copy link
Member

codebrainz left a comment

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).

@elextr

This comment has been minimized.

Copy link
Member

elextr commented Nov 16, 2019

@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

This comment has been minimized.

Copy link
Member Author

b4n commented Nov 17, 2019

@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 --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.

@b4n

This comment has been minimized.

Copy link
Member Author

b4n commented Nov 17, 2019

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.

@codebrainz

This comment has been minimized.

Copy link
Member

codebrainz commented Nov 17, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.