Skip to content

Encode the misc SV_* flags using bitshifts rather than decimal integers #19120

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

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Sep 12, 2021

The bitwise pattern, and the gap at (1<<3), are more visible this way.

@hvds
Copy link
Contributor

hvds commented Sep 12, 2021

This is better than the decimals, but I'd find it easier to read if they were just hex constants (as most of the other flags are, even in sv.h).

Generally the only reason I look at these values is because I'm sitting in gdb trying to check what set of flags a given number corresponds to. It's not a huge effort to convert the shifts into hex mentally, but still more than none.

@khwilliamson
Copy link
Contributor

khwilliamson commented Sep 13, 2021 via email

@nwc10
Copy link
Contributor

nwc10 commented Sep 28, 2021

The goal was:

The bitwise pattern, and the gap at (1<<3), are more visible this way.

more visible than decimal...

So write them as hex?

0x0001
0x0002
0x0004
0x0010
0x0020
0x0040
0x0080
0x0100
0x0200
0x0400
0x0800
0x1000
0x2000
0x4000
0x8000

This missing one is more obvious than decimal?

@demerphq
Copy link
Collaborator

demerphq commented Sep 28, 2021 via email

@khwilliamson
Copy link
Contributor

Is there a reason now to not merge this?

Using decimal integers is confusing, so we show the hex and
decimal values as a comment.

Note: Yves edited the commit message and added the commented
values.
@demerphq
Copy link
Collaborator

demerphq commented Feb 8, 2023

I have rebased this and modified it to show the hex and decimal values as a comment, and force pushed it. I also decided to "document" the unused bit with a define SV_FLAG_BIT3_UNUSED.

I will merge this once it passes CI.

@demerphq demerphq merged commit bd90fa4 into Perl:blead Feb 8, 2023
@iabyn
Copy link
Contributor

iabyn commented Feb 19, 2023 via email

@demerphq
Copy link
Collaborator

and you'd have to look carefully (and know to look) to spot that SV_FLAG_BIT3_UNUSED is actually an unused bit.

Its not a hill I want to die on, so I don't mind removing it. But the contrary view is the it is much easier to manage in terms of ensuring things are well formed, and automating things like showing the value different ways. With a "gap" its harder for a tool to see that the defines are all related, and things like that. I think if we have a well defined convention, then it should be pretty obvious its not a real bit flag. Maybe SV_FLAG_BIT3_UNUSED isnt perfect. It could be: 'XXXXXXX_SV_B3' or something like that. Its easier to teach humans a convention than it is to teach a program about human irregularity.

I would like us to automate more and more of our maintenance. I see lists of defines where the first K have N characters, then the next K have N+1, etc, which IMO makes it hard to read. The more regular we are about these lists the more reasonable it is to write a script that keeps them well formed or to detect when they are not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants