GNOME Bugzilla – Bug 773297
Switch to AX_COMPILER_FLAGS and compile warning-free
Last modified: 2016-10-22 01:44:57 UTC
See https://wiki.gnome.org/Projects/GnomeCommon/Migration Here's a series of patches eliminating all compile warnings in GJS. The idea would be for GJS contributors to develop with --enable-Werror on, while JHbuild would have --disable-Werror by default. The patches are also available on the wip/ptomato/warnings branch.
Created attachment 338142 [details] [review] build: Make a jsapi.h "system header" wrapper We got rid of all the "compat" stuff in compat.h already, before this commit it only included jsapi.h, jsdbgapi.h, and jsapi-util.h, wrapping the former two in some diagnostic pragmas. If we get rid of the jsapi-util.h include, then we can use a system_header pragma to mark it as a system header so that we don't have to painstakingly maintain all those diagnostic pragmas. (The system_header pragma ignores all warnings coming from that file: https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html) Since compat.h already wasn't a fitting name anymore, rename the header to jsapi-wrapper.h.
Created attachment 338143 [details] [review] warnings: Remove unused variables and functions In order to compile everything to the exacting standards of AX_COMPILER_FLAGS, this fixes all instances of -Wunused by removing the unused code.
Created attachment 338144 [details] [review] build: Test for %Id as alternate integer output %Id is a glibc extension. The "I" character is used elsewhere in other extensions too. For example, %I is a Microsoft extension for printing size_t. Although undocumented on Darwin, that platform's libc emulates the Microsoft extension. If %I is interpreted as size_t, then the code is likely to crash on platforms where int and size_t are different widths, such as 64-bit OSX. This adds a configure-time check for %Id. When it's not supported, then the provided sample program will error out when compiled with -Werror. If not supported then we simply print %d since I don't believe the alternative representation flag has any other equivalent on other platforms.
Created attachment 338145 [details] [review] js: Use std::vector instead of GArray C++ does not approve of type-punning GArray's internal char *data pointer. This causes -Wcast-align because the alignment requirements on the underlying array may be different depending on which type it is. Casting char * to int * when the char array does not have int alignment may be slower on some architectures, or it may crash on some, for example older ARM architectures. We use std::vector instead since that does not have the same problem. In cairo-context.cpp we go ahead and switch to RAII style in the function where we replace GArray, since otherwise the gotos would cross the initialization boundary.
Created attachment 338146 [details] [review] byte array: Don't reinterpret char * as jschar * Doing so breaks alignment requirements and may have been the cause of an ARM-only crash we saw on ByteArray a couple years ago. Doing an extra copy seems wasteful, but is the recommended way according to an article on the subject [1], and compilers are sometimes able to optimize out the copy these days... [1] http://blog.regehr.org/archives/959
Created attachment 338147 [details] [review] tests: Fix format strings sscanf() needs a pointer to a char array, not a pointer to a pointer to a char array; and %lli is not the correct format specifier for gint64 on all platforms, but since those struct fields are longs anyway we may as well just print longs.
Created attachment 338148 [details] [review] util: Provide no-op body for empty debug macros In the case where debug macros are disabled, we provide a no-op body. This allows you to do things like if (condition) gjs_debug_something("message"); without the compiler thinking you are doing this if (condition); which it would flag as a warning.
Created attachment 338149 [details] [review] build: Add all enum values to switch statements It's become the recommended GNOME style to compile with -Wswitch-enum and -Wswitch-default; see AX_COMPILER_FLAGS. Per Philip Withnall, when purposely handling only a few possible enum values inside a switch statement, it's better to use if statements. On the other hand, when doing the much more common pattern of default: g_assert_not_reached(); it's better to list exactly which cases should not be reached, as well as guard against invalid values with the default label.
Created attachment 338150 [details] [review] build: Fix various compiler warnings Fixing various compiler warnings that are too small to have their own commit.
Created attachment 338151 [details] [review] build: Switch to AX_COMPILER_FLAGS This gets rid of the last depdenency on gnome-common, switching from GNOME_COMPILE_WARNINGS to AX_COMPILER_FLAGS. See: https://wiki.gnome.org/Projects/GnomeCommon/Migration
Interesting, _Pragma("GCC system_header") works on Clang but not on GCC, whereas the #pragma variant works correctly...
Created attachment 338232 [details] [review] build: Make a jsapi.h "system header" wrapper We got rid of all the "compat" stuff in compat.h already, before this commit it only included jsapi.h, jsdbgapi.h, and jsapi-util.h, wrapping the former two in some diagnostic pragmas. If we get rid of the jsapi-util.h include, then we can use a system_header pragma to mark it as a system header so that we don't have to painstakingly maintain all those diagnostic pragmas. (The system_header pragma ignores all warnings coming from that file: https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html) Since compat.h already wasn't a fitting name anymore, rename the header to jsapi-wrapper.h.
Created attachment 338233 [details] [review] build: Fix various compiler warnings Fixing various compiler warnings that are too small to have their own commit.
Review of attachment 338143 [details] [review]: Sure
Review of attachment 338144 [details] [review]: OK
Review of attachment 338145 [details] [review]: OK
Review of attachment 338146 [details] [review]: OK
Review of attachment 338147 [details] [review]: Looks good.
Review of attachment 338148 [details] [review]: OK
Review of attachment 338149 [details] [review]: Looks good.
Review of attachment 338151 [details] [review]: Nice!
Review of attachment 338232 [details] [review]: OK
Review of attachment 338233 [details] [review]: Sure
Wow, that was faster and less controversial than I expected. Thanks for the quick review! Attachment 338143 [details] pushed as f8482cb - warnings: Remove unused variables and functions Attachment 338144 [details] pushed as be2cf98 - build: Test for %Id as alternate integer output Attachment 338145 [details] pushed as f87aaa9 - js: Use std::vector instead of GArray Attachment 338146 [details] pushed as f96a98c - byte array: Don't reinterpret char * as jschar * Attachment 338147 [details] pushed as 7ad4a80 - tests: Fix format strings Attachment 338148 [details] pushed as 882a8da - util: Provide no-op body for empty debug macros Attachment 338149 [details] pushed as 4c33fe5 - build: Add all enum values to switch statements Attachment 338151 [details] pushed as bef3138 - build: Switch to AX_COMPILER_FLAGS Attachment 338232 [details] pushed as 8c62000 - build: Make a jsapi.h "system header" wrapper Attachment 338233 [details] pushed as 7a1d069 - build: Fix various compiler warnings