config_gnome3: support glib 2.78+ g_free_sized - #133
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Ubuntu workflow adds three Ubuntu 26.04 Clang build variants. GNOME 3 configuration conditionally resolves and uses GLib’s size-aware freeing function for allocated strings. ChangesUbuntu 26.04 build matrix
GNOME 3 size-aware freeing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #133 +/- ##
==========================================
+ Coverage 58.57% 63.91% +5.34%
==========================================
Files 32 34 +2
Lines 2607 2918 +311
Branches 526 546 +20
==========================================
+ Hits 1527 1865 +338
+ Misses 745 707 -38
- Partials 335 346 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
18df4f9 to
aa98740
Compare
aa98740 to
6db4285
Compare
|
Rebased for re-running the checks: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config_gnome3.c`:
- Around line 37-48: The fallback branch must define g_free, not gfree, and the
wrapper must be installed regardless of whether GLib already defines g_free as a
macro. Update the preprocessor structure around the g_free wrapper so any
existing definition is removed when present, then define g_free to call
g_proxy_config_gnome3.g_free in builds without sized-free support.
- Around line 183-188: Update the g_free_sized resolution in the GNOME3
initialization flow to be optional: when dlsym cannot resolve it, assign the
already-resolved g_free function instead of jumping to gnome3_init_error.
Preserve the existing g_free_sized lookup and conditional compilation while
allowing backend initialization to continue on older GLib runtimes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cdbebd9-2164-4f6f-a149-e14e5a0c7d9a
📒 Files selected for processing (2)
.github/workflows/ubuntu.yamlconfig_gnome3.c
| #ifdef g_free | ||
| # undef g_free | ||
| # if G_GNUC_CHECK_VERSION(4, 1) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_78 && defined(G_HAVE_FREE_SIZED) | ||
| # define g_free(mem) \ | ||
| (__builtin_object_size((mem), 0) != ((size_t)-1)) \ | ||
| ? (g_proxy_config_gnome3.g_free_sized)(mem, __builtin_object_size((mem), 0)) \ | ||
| : (g_proxy_config_gnome3.g_free)(mem) | ||
| # else | ||
| # define gfree(mem) (g_proxy_config_gnome3.g_free)(mem) | ||
| # endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== config_gnome3.c relevant lines ==\n'
nl -ba config_gnome3.c | sed -n '1,220p'
printf '\n== search for gfree / g_free references ==\n'
rg -n '\bgfree\b|\bg_free\b|G_HAVE_FREE_SIZED|GLIB_VERSION_MAX_ALLOWED' config_gnome3.cRepository: snxd/proxyres
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== surrounding context for call sites ==\n'
nl -ba config_gnome3.c | sed -n '48,170p'Repository: snxd/proxyres
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the file exists elsewhere, locate it and inspect exact sections.
file="$(git ls-files '*config_gnome3.c' | head -n 1)"
printf 'file=%s\n' "$file"
nl -ba "$file" | sed -n '1,220p'Repository: snxd/proxyres
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== config_gnome3.c relevant lines ==\n'
sed -n '1,220p' config_gnome3.c
printf '\n== search for gfree / g_free references ==\n'
grep -nE '\bgfree\b|\bg_free\b|G_HAVE_FREE_SIZED|GLIB_VERSION_MAX_ALLOWED' config_gnome3.cRepository: snxd/proxyres
Length of output: 9392
Fix the g_free fallback macro The #else branch currently defines gfree, so g_free(...) falls back to the GLib symbol instead of g_proxy_config_gnome3.g_free on builds without sized-free support. The outer #ifdef g_free also skips the wrapper entirely unless GLib exposes g_free as a macro.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config_gnome3.c` around lines 37 - 48, The fallback branch must define
g_free, not gfree, and the wrapper must be installed regardless of whether GLib
already defines g_free as a macro. Update the preprocessor structure around the
g_free wrapper so any existing definition is removed when present, then define
g_free to call g_proxy_config_gnome3.g_free in builds without sized-free
support.
There was a problem hiding this comment.
And what do you propose?
There was a problem hiding this comment.
@nmoinvaz, here's the concrete fix — the wrapper should always be installed (not just when glib already defines g_free as a macro), and the fallback branch should define g_free, not gfree:
`#ifdef` g_free
# undef g_free
`#endif`
`#if` G_GNUC_CHECK_VERSION(4, 1) && GLIB_VERSION_MAX_ALLOWED >= GLIB_VERSION_2_78 && defined(G_HAVE_FREE_SIZED)
# define g_free(mem) \
(__builtin_object_size((mem), 0) != ((size_t)-1)) \
? (g_proxy_config_gnome3.g_free_sized)(mem, __builtin_object_size((mem), 0)) \
: (g_proxy_config_gnome3.g_free)(mem)
`#else`
# define g_free(mem) (g_proxy_config_gnome3.g_free)(mem)
`#endif`Two changes from the current code:
- Removed the outer
#ifdef g_freegate around the whole block. In most glib builds,g_freeis declared as a plain function (not a macro), so the current#ifdef g_freecheck is false and the wrapper never activates — meaning sized-free is silently skipped even on systems that support it, and the wrapper is generally moot.#undefis a safe no-op ifg_freeisn't currently a macro, so it's fine to always attempt it before defining our own macro. - Fixed the typo in the fallback branch:
#define gfree(mem) ...→#define g_free(mem) ..., since the misspelled name meant that on builds without sized-free support, calls tog_free(...)in this file wouldn't route throughg_proxy_config_gnome3.g_freeat all (they'd resolve to the real GLibg_free, which wasn't loaded viadlsym/is separately linked, potentially bypassing the dlopen'd module design).
Let me know if you'd like me to open a PR-scoped follow-up or if you want to apply this directly.
6db4285 to
cc1eccd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config_gnome3.c (1)
37-47: 🎯 Functional Correctness | 🟠 MajorStill remove the outer
#ifdef g_freeguard.GLib declares
g_freeas a function and only conditionally defines the macro. When that macro is absent, this block is skipped entirely, so the new call sites bypass thedlsym-resolved function and may reintroduce a direct link dependency. (docs.gtk.org)Move the
#ifdefaround only#undef g_free, then define the fallback wrapper unconditionally.Proposed fix
-#ifdef g_free -# undef g_free -# if ... +#ifdef g_free +# undef g_free +#endif +#if ... ... -# endif `#endif`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config_gnome3.c` around lines 37 - 47, Update the g_free compatibility block so the outer `#ifdef` g_free guards only `#undef` g_free, then define the replacement g_free wrapper unconditionally using the existing version check and dlsym-resolved proxy functions. Ensure call sites use g_proxy_config_gnome3.g_free or g_free_sized even when GLib did not originally define a g_free macro.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@config_gnome3.c`:
- Around line 37-47: Update the g_free compatibility block so the outer `#ifdef`
g_free guards only `#undef` g_free, then define the replacement g_free wrapper
unconditionally using the existing version check and dlsym-resolved proxy
functions. Ensure call sites use g_proxy_config_gnome3.g_free or g_free_sized
even when GLib did not originally define a g_free macro.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04261c87-c26f-4125-8d73-1edf9953666c
📒 Files selected for processing (2)
.github/workflows/ubuntu.yamlconfig_gnome3.c
There was a problem hiding this comment.
Pull request overview
This PR updates the GNOME3 configuration backend to cope with newer GLib memory-free APIs/macros, and expands CI to validate builds on Ubuntu 26.04 across multiple feature configurations.
Changes:
- Add a
g_freewrapper strategy inconfig_gnome3.cto handle environments where GLib providesg_freeas a macro and may useg_free_sized. - Extend the Ubuntu GitHub Actions workflow matrix with Ubuntu 26.04 variants (default, curl, duktape) and corresponding llvm-cov settings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config_gnome3.c | Adds support for GLib g_free_sized / g_free macro interactions by routing frees through dynamically resolved symbols. |
| .github/workflows/ubuntu.yaml | Adds Ubuntu 26.04 build+test+coverage jobs for multiple configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nmoinvaz
changed the title
cc1eccd to
357e5d7
Compare
nmoinvaz
changed the title
357e5d7 to
de7ec2d
Compare
|
@sergio-nsk I cleaned up the PR title and commit message and also split it into 2 commits. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config_gnome3.c`:
- Around line 37-47: Update the g_free wrapper near g_proxy_config_gnome3 so
`#undef` g_free is guarded independently, then define the custom g_free macro
unconditionally. Preserve the existing conditional sized-free dispatch when its
guard is true, and otherwise route all calls through
g_proxy_config_gnome3.g_free rather than requiring a direct GLib symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b79e7f21-08b8-4595-ac07-a37c4b89f4cd
📒 Files selected for processing (2)
.github/workflows/ubuntu.yamlconfig_gnome3.c
Summary by CodeRabbit
Bug Fixes
Chores