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

Fix header files #109

Merged
merged 1 commit into from
Aug 14, 2021
Merged

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Aug 7, 2021

json.h: <inttypes.h><stdint.h>
int64_t is defined in <stdint.h>, no need for the full <inttypes.h>

json.h: <stdlib.h><stddef.h>
<stddef.h> is enough to define size_t

json.c: <stdlib.h>
Now that <stdlib.h> has been removed from json.h, add it to json.c for malloc()/calloc() and free()

json.h: <inttypes.h> → <stdint.h>
int64_t is defined in <stdint.h>, no need for the full <inttypes.h>

json.h: <stdlib.h> → <stddef.h>
<stddef.h> is enough to define size_t

json.c: <stdlib.h>
Now that <stdlib.h> has been removed from json.h, add it to json.c for
malloc()/calloc() and free()
@DimitriPapadopoulos
Copy link
Contributor Author

Note that this patch does not change the status of the library regarding ANSI C vs. C89 compatibility:

  • The additional <stddef.h> header is ANSI C.
  • The ANSI C header <stdlib.h> is just moved around, from json.h to json.c.
  • Neither <inttypes.h> nor <stdint.h> are ANSI C. Both are C99 and both are included only if json_int_t is not already defined, and additionally <inttypes.h> includes <stdint.h> anyway:
#ifndef json_int_t
   #ifndef _MSC_VER
      #include <inttypes.h>

@LB-- LB-- added this to the v1.1.1 milestone Aug 14, 2021
Copy link
Member

@LB-- LB-- left a comment

Choose a reason for hiding this comment

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

Thanks! After doing some more thinking and research, perhaps it would be better to use a progressive-enhancement approach where we check the presence and value of __STDC_VERSION__ to know what is safe to use. I think also we should stick to long/long long/int_fast64_t since int64_t and co are optional and not guaranteed to exist. Though, that might be better suited to a different commit, as there are more places than just here that use int64_t. The user can also override the default by defining json_int_t, which I hope to add proper documentation for later. Thoughts?

Also, would you like your name added to the AUTHORS file? You may add it yourself or I will add it for you, or you can opt-out, whichever you prefer.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Aug 14, 2021

I agree:

  • check __STDC_VERSION__ to introduce C99 headers for example,
  • avoid int64, but on the other hand int64 is used only if json_int_t is not defined.

However, I also agree the above ideas are orthogonal to the current patch, which is about using the proper header files without changing the status with respect to ANSI C or int64. I would rather see this pull request merged before working on the above ideas.

@LB--
Copy link
Member

LB-- commented Aug 14, 2021

without changing the status

Good point, I'll go ahead and merge this PR in that case. Yes or no to AUTHORS file?

@DimitriPapadopoulos
Copy link
Contributor Author

I've submitted another PR for AUTHORS.

LB-- added a commit to LB--/json-parser that referenced this pull request Aug 14, 2021
@LB-- LB-- merged commit 1130ad2 into json-parser:master Aug 14, 2021
@DimitriPapadopoulos DimitriPapadopoulos deleted the include_fixes branch August 14, 2021 20:35
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

2 participants