NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

GH-98686: Fix compiler warning for`HAS_ARG`#99106

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

brandtbucher merged 2 commits intopython:mainfrombrandtbucher:has-arg-warning

Nov 4, 2022

Merged

GH-98686: Fix compiler warning for`HAS_ARG`#99106

brandtbucher merged 2 commits intopython:mainfrombrandtbucher:has-arg-warning

Nov 4, 2022

Conversation

Copy link

Member

brandtbucher commentedNov 4, 2022•
edited by bedevere-bot
Loading

Issue:Quicken everything #98686

brandtbucher added2 commits

October 20, 2022 11:09

Fix compiler warning

739a763

Just cast instead

ce1d3d6

brandtbucher added skip news interpreter-core(Objects, Python, Grammar, and Parser dirs) labels

Nov 4, 2022

brandtbucher requested a review fromiritkatriel

November 4, 2022 20:00

brandtbucher requested a review frommarkshannon as acode owner

November 4, 2022 20:00

brandtbucher self-assigned this

Nov 4, 2022

bedevere-bot mentioned this pull request

Nov 4, 2022

Quicken everything#98686

Open

bedevere-bot added the awaiting core review label

Nov 4, 2022

iritkatriel reviewed

Nov 4, 2022

View reviewed changes

Python/ceval.c Outdated

		@@ -143,7 +143,7 @@ lltrace_instruction(_PyInterpreterFrame *frame,
		const char *opname = _PyOpcode_OpName[opcode];
		assert(opname != NULL);
		int offset = (int)(next_instr - _PyCode_CODE(frame->f_code));
		if (HAS_ARG(_PyOpcode_Deopt[opcode])) {
		if (HAVE_ARGUMENT <=_PyOpcode_Deopt[opcode]) {

Copy link

Member

Choose a reason for hiding this comment

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

HAVE_ARGUMENT isn't used directly anywhere else. I think casting and using HAS_ARG might be better.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

But we don't have to worry about things like pseudo-opcodes, right? This seems like exactly the sort of thingHAVE_ARGUMENT is meant for...

Copy link

Member

Choose a reason for hiding this comment

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

Kind of. HAVE_ARGUMENT was there before we had pseudo-opcodes, and HAS_ARG just compared with it. Now we have pseudo-opcodes, and if I saw HAVE_ARGUMENT in the code now my first thought would be that this is a bug (a place where we didn't update for pseudo-opcodes). So the trick is to make this not look like a bug.