Skip to content

Conversation

@jkeenan
Copy link
Contributor

@jkeenan jkeenan commented Aug 21, 2023

This suppresses the -Wunused_variable warning reported in #21393 -- but since I'm just monkey-patching, please review.

(You may get a porting test failure because (I think) Module-CoreList needs to be updated after today's release.)

@jkeenan jkeenan requested review from iabyn and leonerd August 21, 2023 01:08
pp_ctl.c Outdated
SAVETMPS;
PUSHMARK(PL_stack_sp);
rpp_xpush_1(name_sv); /* always use the object for method calls */
int count = call_sv(PL_hook__require__before, G_SCALAR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it, the type of count should be changed from int to I32 because that's what call_sv returns.

@iabyn
Copy link
Contributor

iabyn commented Aug 21, 2023

Really the count variable and assertion should just be removed, and call_sv() be called without using it's return value. The code I changed had 'if (count && ...)', but call_sv(...,G_SCALAR) always returns 1. The assertion was just a temporary crutch to check that I wasn't changing behaviour when I removed the 'count > 0' check.

@jkeenan jkeenan force-pushed the smoke-me/jkeenan/gh-21293-unused-var branch from 32a5994 to 115a10b Compare August 21, 2023 12:33
@jkeenan jkeenan changed the title Smoke me/jkeenan/gh 21293 unused var Smoke me/jkeenan/gh 21393 unused var Aug 21, 2023
@jkeenan
Copy link
Contributor Author

jkeenan commented Aug 21, 2023

Really the count variable and assertion should just be removed, and call_sv() be called without using it's return value. The code I changed had 'if (count && ...)', but call_sv(...,G_SCALAR) always returns 1. The assertion was just a temporary crutch to check that I wasn't changing behaviour when I removed the 'count > 0' check.

See latest version of this pull request.

@jkeenan jkeenan added squash-before-merge Author must squash the commits down before merging to blead build-time-warnings Replaces [META] Build-time warnings RT #133556 labels Aug 21, 2023
@iabyn
Copy link
Contributor

iabyn commented Aug 21, 2023

LGTM

Implement Dave Mitchell's suggestion: we don't really need the unused
variable.

For: #21393
@jkeenan jkeenan force-pushed the smoke-me/jkeenan/gh-21293-unused-var branch from 115a10b to bc6e83c Compare August 21, 2023 13:07
@jkeenan jkeenan merged commit 1892bf8 into blead Aug 21, 2023
@jkeenan jkeenan deleted the smoke-me/jkeenan/gh-21293-unused-var branch August 21, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-time-warnings Replaces [META] Build-time warnings RT #133556 squash-before-merge Author must squash the commits down before merging to blead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants