-
Notifications
You must be signed in to change notification settings - Fork 169
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
[Build] Add g_memdup2() support for glib >= 2.68 #3550
Conversation
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #3550. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://nnstreamer.mooo.com/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvuillaumier, 💯 All CI checkers are successfully verified. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can define a macro
#if GLIB_USE_G_MEMDUP2
#define _g_memdup(data, size) g_memdup2 (data, size)
#else
#define _g_memdup(data, size) g_memdup (data, size)
#endif
in a common header (e.g., nnstreamer_pluigin_api.h).
Let's have minimum #if/#endif in .c/.cc files.
ec6b3fc
to
5eb703d
Compare
Changeset updated accordingly: |
meson.build
Outdated
if (glib_dep.found()) | ||
add_project_arguments('-DGLIB_USE_G_MEMDUP2', language: ['c', 'cpp']) | ||
else | ||
glib_dep = dependency('glib-2.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using macro in header file or version_compare() in meson
Use glib macro in header
#if GLIB_CHECK_VERSION (2, 68, 0)
#define _g_memdup g_memdup2
#else
#define _g_memdup g_memdup
#endif
or meson.build
if glib_dep.version().version_compare('>= 2.68.0')
add_project_arguments('-DGLIB_USE_G_MEMDUP2', language: ['c', 'cpp'])
endif
IMO, 1st one is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with dropping meson.build change and go for GLIB_CHECK_VERSION() check in the header file if all agree with that.
However doing so requires addition of
#include <glib.h>
at the top of the header file so that the macro is guaranteed to be defined.
May not be a bid deal but my understanding is that at the moment .c/.cc files include glib.h explicitly, not indirectly via other header files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first approach (no changes in meson) looks more beautiful, but we cannot ensure the order of #include
statements; the coding-style checks (gst-indent, clang-format, ...) often force to reorder them and having such implicit conditions don't feel right.
Let's proceed with the second approach (adding a few lines in meson.build).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patchset updated with suggestion from @jaeyun-jung for option 2 in meson.build:
if glib_dep.version().version_compare('>= 2.68.0') add_project_arguments('-DGLIB_USE_G_MEMDUP2', language: ['c', 'cpp']) endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvuillaumier, 💯 All CI checkers are successfully verified. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update to fix build with glib version 2.68 and later Legacy function: gpointer g_memdup ( gconstpointer mem, guint byte_size ) is deprecated since version 2.68 and replaced by: gpointer g_memdup2 ( gconstpointer mem, gsize byte_size ) Difference is byte_size argument type changed from guint to gsize. Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
5eb703d
to
9a9fdce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvuillaumier, 💯 All CI checkers are successfully verified. Thanks.
Function g_memdup() is deprecated starting from glib 2.68, replaced by g_memdup2() :
" Use g_memdup2() instead, as it accepts a #gsize argument for byte_size, avoiding the possibility of overflow in a #gsize → #guint conversion."
https://docs.gtk.org/glib/func.memdup.html
To keep compatibility with the 2 g_memdupX() variants, option used is to detect glib version from meson script.
Alternative option may be to use in the source code construct like:
`
#ifdef GLIB_CHECK_VERSION(2, 68, 0)
data = g_memdup2(...);
#else
data = g_memdup(...);
#endif
`
Self Evaluation:
SSAT tests: passed
Update to fix build with glib version 2.68 and later
Legacy function:
gpointer g_memdup ( gconstpointer mem, guint byte_size )
is deprecated since version 2.68 and replaced by:
gpointer g_memdup2 ( gconstpointer mem, gsize byte_size )
Difference is byte_size argument type changed from guint to gsize.
Signed-off-by: Julien Vuillaumier julien.vuillaumier@nxp.com