Skip to content
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

Make jsmntype_t values bit flags #108

Merged
merged 5 commits into from Aug 27, 2021
Merged

Make jsmntype_t values bit flags #108

merged 5 commits into from Aug 27, 2021

Conversation

olmokramer
Copy link
Contributor

@olmokramer olmokramer commented Apr 23, 2017

This way one can easily check whether a token is either of multiple types, e.g.:

if (token.type & (JSMN_ARRAY | JSMN_OBJECT))
	// do stuff with array or object

This way one can easily check whether a token is one of a few types:

if (token.type & (JSMN_ARRAY | JSMN_OBJECT))
	// do stuff with array or object
Copy link

@incrediblejr incrediblejr left a comment

Choose a reason for hiding this comment

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

This does not work as the bitshifting is wrong. To achieve the pullrequests intent, you could instead define the enum like:

typedef enum {
JSMN_UNDEFINED = (1<<0),
JSMN_OBJECT = (1<<1),
JSMN_ARRAY = (1<<2),
JSMN_STRING = (1<<3),
JSMN_PRIMITIVE = (1<<4)
} jsmntype_t;

@olmokramer
Copy link
Contributor Author

Oops, my bad! Guess that's what you get for doing a pull request through github's online interface... Thanks for spotting, fixed it in 2nd commit.

@pt300
Copy link
Collaborator

pt300 commented May 12, 2019

I still think it's an interesting concept that's worth considering. It can't be pulled at this point but I'll keep this open just so I won't easily forget about it.

@olmokramer
Copy link
Contributor Author

Don't know why the PR couldn't be merged anymore... Fixed the "conflicts" so it can be pulled again.

@pt300
Copy link
Collaborator

pt300 commented May 12, 2019

This goes into my bucket of things to actually add to jsmn. Thanks for the fix

@olmokramer
Copy link
Contributor Author

olmokramer commented May 13, 2019

Awesome, thanks! Didn't actually change anything in the merge commit I think...

PS Oh wait, looks like indentation whitespace has changed.

@alexhenrie
Copy link

I want this feature too, but please leave JSMN_UNDEFINED as 0, otherwise if (token->type) will no longer be the same as if (token->type != JSMN_UNDEFINED). The definition of jsmntype_t should be:

typedef enum {
  JSMN_UNDEFINED = 0,
  JSMN_OBJECT = 1 << 0,
  JSMN_ARRAY = 1 << 1,
  JSMN_STRING = 1 << 2,
  JSMN_PRIMITIVE = 1 << 3
} jsmntype_t;

@alexhenrie
Copy link

I want this feature because on some architectures (such as ARM), less instructions are required for if (token->type & (JSMN_ARRAY | JSMN_PRIMITIVE)) than for if (token->type == JSMN_ARRAY || token->type == JSMN_PRIMITIVE). Redefining jsmntype_t to resemble a set of bit flags will allow code that uses it to take advantage of that shortcut.

@olmokramer
Copy link
Contributor Author

@alexhenrie that's a good point about leaving JSMN_UNDEFINED = 0. I've reverted the change to JSMN_UNDEFINED.

@pt300
Copy link
Collaborator

pt300 commented Aug 27, 2021

Something similar is already available in the experimental branch, @alexhenrie. Due to extremely slow development there I will just merge this pull request with the main branch.

I do not see any possible incompatibilities at this point. This definition of jsmntype is compatible with any usage I'm aware of and also brings in possibility to test more efficiently for multiple types.

@pt300
Copy link
Collaborator

pt300 commented Aug 27, 2021

There are no issues introduced based on the tests either.

@pt300 pt300 merged commit 23f13d2 into zserge:master Aug 27, 2021
@alexhenrie
Copy link

Thank you very much!

tsupplis added a commit to tsupplis/jsmn that referenced this pull request Aug 30, 2021
Merge pull request zserge#108 from olmokramer/patch-1
@olmokramer olmokramer mentioned this pull request Aug 30, 2021
@aloisklink
Copy link

I do not see any possible incompatibilities at this point. This definition of jsmntype is compatible with any usage I'm aware of and also brings in possibility to test more efficiently for multiple types.

The only possible incompatibility is that this is an ABI breaking change. But since jsmn is a header-only library, I'm not really sure if ABI breaking changes matter? 🤷

Still, it might be worth making the next release of jsmn a v2.0.0 release, just to be on the safe side. It looks like there are some OpenSUSE packages that are bundling jsmn is a pre-compiled binary, so otherwise they might be affected, see https://pkgs.org/search/?q=jsmn.

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.

None yet

5 participants