Restore the accurate libobs effect-caching comment - #250
Merged
Conversation
…omment
A review finding claimed gs_effect_create_from_file() allocates a fresh
effect per call, and I acted on it before it had been checked against
libobs. It was wrong, and the resulting comments asserted the opposite of
the truth — one of them explicitly telling the next reader that the
earlier, correct version had been mistaken.
Verified in libobs source this time:
graphics.c gs_effect_create_from_file() consults find_cached_effect()
first and returns the existing effect; gs_effect_create()
sets cached = true for any file-created effect.
effect.c gs_effect_destroy() guards the real free with
if (!effect->cached), so destroying a cached effect is a
no-op until obs_free_graphics().
So the Tiles wall and the Loudness Meter share one gs_effect_t*, each
destroy is a no-op, and there is neither a double compile nor a
double-free. Both comments now carry the source references so this stops
flip-flopping.
Comments only; no behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WqDL6gcoVvEMXv7Qwcni9n
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review finding during #249 claimed
gs_effect_create_from_file()allocates a fresh effect on every call. I acted on it before it had been verified against libobs, and the resulting comments asserted the opposite of the truth — one of them explicitly telling the next reader that the earlier, correct version had been mistaken. That is worse than the original, since it actively steers someone away from correct behaviour.The reviewer subsequently checked the actual source and retracted the finding. Verified:
libobs/graphics/graphics.c,gs_effect_create_from_file()— consultsfind_cached_effect(file)first and returns the existing effect;gs_effect_create()setscached = truefor any file-created effect.libobs/graphics/effect.c,gs_effect_destroy()— guards the real free withif (!effect->cached), so destroying a cached effect is a no-op untilobs_free_graphics().So the Tiles wall and the Loudness Meter share one
gs_effect_t*, eachtiles_effect_destroy()is a no-op, and there is neither a double compile nor a double-free nor a leak.Both comments now carry the source references inline so this stops flip-flopping.
Comments only — no behaviour change. Plugin builds clean, 60/60 tests green.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WqDL6gcoVvEMXv7Qwcni9n