4 ms·

parson.c, 109 - wrong conditional Well-written, clean and consistent. A bit too pendantic and therefore too verbose for my taste though :) Why in KnR's name th

by eps 14y ago

parson.c, 109 - wrong conditional Well-written, clean and consistent. A bit too pendantic and therefore too verbose for my taste though :) Why in KnR's name this - JSON_Object *new_obj = (JSON_Object*)parson_malloc(sizeof(JSON_Object)); is not written like this - JSON_Object *new_obj = parson_malloc(sizeof *newobj); On a more serious note - I would move json_*_t structs to the header file and eliminate a bunch of API functions that service the access to these structs. It's a C library after all, everyone knows how to shoot one's own leg. (edit) Oh, and you don't handle realloc failures properly (at least in json_object_add).

deleted 14y ago
[deleted]
kgabis 14y ago
Thanks for pointing out those errors, I already fixed the one with the wrong conditional and will fix the realloc one in the next commit. I'm being explicit with casting, because g++ and vc++ will throw warnings without it.
- eps 14y ago
  Of course you'd get warnings compiling C code with C++ compiler. C doesn't require casting from void* to another pointer type... it's one of its best features :)
  - malkia 14y ago
    Certain people would still like to compile this as a "CPP" file, examples are lua - it's in "C", but it's compilable as C++ too (some people then are more or less strict whether the function interfaces should be extern"C" or not). Ayway, not that important... But there is a whole lot of people that like amalgated sources (typical examples - juce, freetype2, lua, sqlite) where you include in one or two files all the rest and they compile fine.
    - cmccabe 14y ago
      Luckily for you, you can link objects files produced by the C compiler with object files produced by the C++ compiler. Crisis averted.
    - deleted 14y ago
      [deleted]
pflanze 14y ago
> I would move json_* _t structs to the header file and eliminate a bunch of API functions that service the access to these structs. It's a C library after all, everyone knows how to shoot one's own leg. From what I know I would suggest to only do that if the code is never to be used as a shared library, as otherwise the structs can't be changed without recompiling all programs using the library. There was a Debian security update to libxml2 in 2008 that made various Gnome apps including, IIRC, the window manager crash for many users in various circumstances (depending on the used theme etc.)(* ), because it changed some struct. Public API functions were provided in that case, but apparently direct access to structs was not discouraged enough to prevent some programs from going the unsafe route anyway (or perhaps the API wasn't sufficiently complete? I don't know the details). I haven't created any wide spread C programs, and don't know if there are ABI checkers that Debian should use / have used, or if it's really just restraint to procedure based APIs that can avoid such problems. (* ) e.g. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=496125 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=496125 http://lists.debian.org/debian-user/2008/08/msg01864.html http://lists.debian.org/debian-user/2008/08/msg01864.html
- eps 14y ago
  Right. This would apply if parson were built into a shared library and distributed as an .so. It is a possibility, though Debian's package dependency system exists exactly to safeguard against this sort of a situation.
cmccabe 14y ago
"I would move json_*_t structs to the header file and eliminate a bunch of API functions that service the access to these structs. It's a C library after all, everyone knows how to shoot one's own leg." I strongly disagree. The author did exactly the right thing by hiding the internal data representation behind accessor functions. This makes it easy for him to change the internal data representation in the future without breaking binary compatibility. See the libabc REAME for a fuller explanation: https://git.kernel.org/?p=linux/kernel/git/kay/libabc.git;a=blob_plain;f=README https://git.kernel.org/?p=linux/kernel/git/kay/libabc.git;a=... Summary: "Do not expose any complex structures in your API" Unrelated: please don't be one of those people who typedef every struct. It's really ugly and obfuscatory (see Linus' remarks on this).
- deleted 14y ago
  [deleted]

by eps 14y ago

deleted 14y ago

[deleted]

kgabis 14y ago

Thanks for pointing out those errors, I already fixed the one with the wrong conditional and will fix the realloc one in the next commit. I'm being explicit with casting, because g++ and vc++ will throw warnings without it.

eps 14y ago
Of course you'd get warnings compiling C code with C++ compiler. C doesn't require casting from void* to another pointer type... it's one of its best features :)
- malkia 14y ago
  Certain people would still like to compile this as a "CPP" file, examples are lua - it's in "C", but it's compilable as C++ too (some people then are more or less strict whether the function interfaces should be extern"C" or not). Ayway, not that important... But there is a whole lot of people that like amalgated sources (typical examples - juce, freetype2, lua, sqlite) where you include in one or two files all the rest and they compile fine.
  - cmccabe 14y ago
    Luckily for you, you can link objects files produced by the C compiler with object files produced by the C++ compiler. Crisis averted.
  - deleted 14y ago
    [deleted]

pflanze 14y ago

> I would move json_* _t structs to the header file and eliminate a bunch of API functions that service the access to these structs. It's a C library after all, everyone knows how to shoot one's own leg. From what I know I would suggest to only do that if the code is never to be used as a shared library, as otherwise the structs can't be changed without recompiling all programs using the library. There was a Debian security update to libxml2 in 2008 that made various Gnome apps including, IIRC, the window manager crash for many users in various circumstances (depending on the used theme etc.)(* ), because it changed some struct. Public API functions were provided in that case, but apparently direct access to structs was not discouraged enough to prevent some programs from going the unsafe route anyway (or perhaps the API wasn't sufficiently complete? I don't know the details). I haven't created any wide spread C programs, and don't know if there are ABI checkers that Debian should use / have used, or if it's really just restraint to procedure based APIs that can avoid such problems. (* ) e.g. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=496125 http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=496125 http://lists.debian.org/debian-user/2008/08/msg01864.html http://lists.debian.org/debian-user/2008/08/msg01864.html

eps 14y ago
Right. This would apply if parson were built into a shared library and distributed as an .so. It is a possibility, though Debian's package dependency system exists exactly to safeguard against this sort of a situation.

cmccabe 14y ago

"I would move json_*_t structs to the header file and eliminate a bunch of API functions that service the access to these structs. It's a C library after all, everyone knows how to shoot one's own leg." I strongly disagree. The author did exactly the right thing by hiding the internal data representation behind accessor functions. This makes it easy for him to change the internal data representation in the future without breaking binary compatibility. See the libabc REAME for a fuller explanation: https://git.kernel.org/?p=linux/kernel/git/kay/libabc.git;a=blob_plain;f=README https://git.kernel.org/?p=linux/kernel/git/kay/libabc.git;a=... Summary: "Do not expose any complex structures in your API" Unrelated: please don't be one of those people who typedef every struct. It's really ugly and obfuscatory (see Linus' remarks on this).

deleted 14y ago
[deleted]