zserge / jsmn

Jsmn is a world fastest JSON parser/tokenizer. This is the official repo replacing the old one at Bitbucket
MIT License
3.65k stars 778 forks source link

jsmn: declare struct names to allow forward decls #175

Closed pks-t closed 4 years ago

pks-t commented 4 years ago

Both jsmntok_t and jsmn_parser are declared as anonymous structures that are typedeffed to their actual name. This forces all downstream users of jsmn to always use the typedef name, instead of using e.g. struct jsmn_parser. While this might be considered a matter of taste, using typedefs only has the technical downside of disallowing forward declarations. E.g. if a dependent whishes to declare a pointer to jsmntok_t without actually pulling in the "jsmn.h" header, then he is not able to do so because there is no way in C to provide a forward declaration for typedefs to anonymous structs.

Fix this by providing names for both jsmntok_t and jsmn_parser structures.

pt300 commented 4 years ago

This been laying around for so long. Sorry about that. For the most part I would just merge this pull request, although it made me think a little about the naming conventions used in JSMN. How come jsmntok has the _t postfix yet jsmn_parser does not? Having this ironed out I have no reason to keep this pull request waiting.

pt300 commented 4 years ago

Don't get me wrong, the burden of that is not on you, @pks-t. I just want to make sure something as silly as this won't become a problem later on.

pks-t commented 4 years ago

This been laying around for so long. Sorry about that.

No worries, I know it's hard sometimes to keep up :)

For the most part I would just merge this pull request, although it made me think a little about the naming conventions used in JSMN. How come jsmntok has the _t postfix yet jsmn_parser does not? Having this ironed out I have no reason to keep this pull request waiting.

I'm not entirely sure what you propose. Do you propose that I keep the same struct names as the typedeffed ones (that is, using struct jsmntok_t instead of struct jsmntok, even though it's inconsistent with struct jsmn_parser)?

zserge commented 4 years ago

Well, if only I could return 10 years back and rewrite the original code. :see_no_evil: We all know that POSIX explicitly forbids ending your own types with _t. But historically it was used for opaque typedefs. So my original code, clearly, makes no sense - tokens are not opaque at all, and must remain regular struct jsmntok type (or better struct jsmn_tok?), to give a hint that one will have to access its fields, and to match POSIX recommendations. With this in mind, struct jsmn_parser is also OK, but might have been aliased as jsmn_parser_t, although I see little point in it.

Would it be too brutal if we abandon _t suffices and leave it plain unaliased structs? I honestly don't see how to fix a broken thing without making a breaking change.

pt300 commented 4 years ago

I still have in mind quite breaking changes for a future release, so the change of names might be added to the todo list for when that happens. For now, it probably would be smart to just drop the _t in the name of the structure and keep it in the typedef, exactly the way it's done in the pull request. This could in theory make it less painful to adjust code when we do finally drop the _t postfix. Although, that could be not the case if name gets changed to jsmn_tok. For now I'd give this change a pass.

pks-t commented 4 years ago

Just to make sure I'm not missing anything: do you expect me to make any changes right now?

pt300 commented 4 years ago

Not really. Seeing that @zserge seems to be alright with it too then into the code it goes. The naming probably will be adjusted in the future but it's not a concern for now.

pks-t commented 4 years ago

Cool, thanks a lot!