-
Notifications
You must be signed in to change notification settings - Fork 186
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
Prevent warnings from reminder definitions defined in civetweb thridparty #378
Conversation
8d667aa
to
272b44a
Compare
Rebased over master to extend the tests on the fork. The PR seems still necessary (or a similar fix that manages to suppress the warnings). Using e.g. |
Thank you for the suggestion, but I prefer to not change any third-party files (Civetweb, WebView2), because we want to let user update those files if needed, like running I tried myself something like: #if defined(__GNUC__) || defined(__TINYC__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wsign-compare"
#include "webview/WebView2.h"
#pragma GCC diagnostic pop
#else
#include "webview/WebView2.h"
#endif But it does not work for some reason... only putting Let's find a way to suppress the third-party warnings without any manual edit first, if not, then we do like you suggested. |
|
I see your point, and would also prefer not to change them. But sometimes it's inevitable to make minor adaptions in third-party code when working with it. Then it just can be documented what changes need to be made when the libraries are updated. E.g. https://github.com/facebook/zstd is used as a thirdparty module in V. When it gets updated the definition below needs to get added manually. Thirdpary updates don't happen to often, so as long there are no drastic changes it's little effort to manage. #if defined(__TINYC__) && defined(\_WIN32)
#undef ZSTD_MULTITHREAD
#define ZSTD_NO_INTRINSICS
#endif When opening this PR I also submitted a PR at civetweb simultaneously. Maybe it gets integrated over there soon, then this condition comes nativeley with civetweb. |
Fair enough. Let's fine out if we can add something generic in the top of the third-party files to completely supress all warnings. |
Hopefully 67179f5 will fix this issue. |
Nope, still when trying to interact with WebUI directly.
The changes might still be a viable way. Best would be if they merge it over a civetweb. There is no activity atm. Also, would it be good to add a condition to surpess the warnings to keep the possibility to get more information from the compiler during development? |
Sorry, I only focused on building |
This prevents warning from definitions in
civetweb.c
that are meant as reminders.webui/src/civetweb/civetweb.c
Lines 1534 to 1535 in ce3dce4
Those civetweb definitions can also trigger unintended for the uses
malloc
andfree
in webui.E.g., they prevent to directly interact with the WebUI C code from CGO.
On the left an example CI run on a Go fork. On the right using webui with the civetweb.c changes in this PR can sucessfully compile:
The fix is achieved by wrapping the reminders in a
NDEBUG
condition which is a definition we already add during compiling.