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

Prevent warnings from reminder definitions defined in civetweb thridparty #378

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented May 30, 2024

This prevents warning from definitions in civetweb.c that are meant as reminders.

webui/src/civetweb/civetweb.c

Lines 1534 to 1535 in ce3dce4

/* This following lines are just meant as a reminder to use the mg-functions
* for memory management */

Those civetweb definitions can also trigger unintended for the uses malloc and free 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:

Screenshot_20240530_191931 Screenshot_20240530_192627

The fix is achieved by wrapping the reminders in a NDEBUG condition which is a definition we already add during compiling.

@ttytm ttytm force-pushed the prevent-civetweb-warnings branch from 8d667aa to 272b44a Compare May 30, 2024 21:52
@ttytm
Copy link
Member Author

ttytm commented May 30, 2024

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. #undef DO_NOT_USE_THIS_FUNCTION__USE_mg_realloc for all the civetweb definitions unfortunately doesn't work. The condition might be a slim way to solve it.

@hassandraga
Copy link
Member

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 nuget to get latest WebView2, or integrate WebUI in a project where WebView2 is taken from Windows SDK. Another reason is we will need to make those manual edit each time we update the third-party files.

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 #pragma GCC system_header in the top of third-party files by a manual edit like you suggested work, but I still skeptical because of the reasons above.

Let's find a way to suppress the third-party warnings without any manual edit first, if not, then we do like you suggested.

@hassandraga
Copy link
Member

-isystem does not work either.

@ttytm
Copy link
Member Author

ttytm commented Jun 1, 2024

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.

@hassandraga
Copy link
Member

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.

@hassandraga
Copy link
Member

Hopefully 67179f5 will fix this issue.

@hassandraga hassandraga closed this Jun 5, 2024
@ttytm
Copy link
Member Author

ttytm commented Jun 6, 2024

Nope, still when trying to interact with WebUI directly.

❯ go run ./examples/minimal.go
# github.com/webui-dev/go-webui/v2
In file included from ./cgo.go:16:
./webui/src/webui.c: In function ‘_webui_is_firefox_ini_profile_exist’:
./webui/src/civetweb/civetweb.c:1558:18: error: implicit declaration of function ‘DO_NOT_USE_THIS_FUNCTION__USE_mg_snprintf’ [-Wimplicit-function-declaration]
 1558 | #define snprintf DO_NOT_USE_THIS_FUNCTION__USE_mg_snprintf
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./webui/src/webui.c:1103:13: note: in expansion of macro ‘snprintf’
 1103 |             snprintf(full_path, sizeof(full_path), "%s/%s", home, & path[1]);
      |             ^~~~~~~~
./webui/src/webui.c: In function ‘_webui_free_mem’:
./webui/src/civetweb/civetweb.c:1557:14: error: implicit declaration of function ‘DO_NOT_USE_THIS_FUNCTION__USE_mg_free’ [-Wimplicit-function-declaration]
 1557 | #define free DO_NOT_USE_THIS_FUNCTION__USE_mg_free
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./webui/src/webui.c:3124:13: note: in expansion of macro ‘free’
 3124 |             free(ptr);
      |             ^~~~
./webui/src/webui.c: In function ‘_webui_malloc’:
./webui/src/civetweb/civetweb.c:1554:16: error: implicit declaration of function ‘DO_NOT_USE_THIS_FUNCTION__USE_mg_malloc’ [-Wimplicit-function-declaration]
 1554 | #define malloc DO_NOT_USE_THIS_FUNCTION__USE_mg_malloc
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./webui/src/webui.c:3216:17: note: in expansion of macro ‘malloc’
 3216 |         block = malloc(size);
      |                 ^~~~~~
In file included from ./cgo.go:18:
./webui/src/webui.c:3216:15: error: assignment to ‘void *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
 3216 |         block = malloc(size);
      |               ^

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?

@hassandraga
Copy link
Member

Sorry, I only focused on building webui.c without warnings from civetweb.h.
I forgot civetweb.c. Let me fix this.

@hassandraga hassandraga reopened this Jun 6, 2024
@hassandraga hassandraga merged commit 1e6196c into webui-dev:main Jun 6, 2024
37 checks passed
@ttytm ttytm deleted the prevent-civetweb-warnings branch June 6, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants