Re: [PATCH v2 2/6] json: Implement wrapping interface

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> On 11/06, Patrick Steinhardt wrote:
> > On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote:
> > > I had one more comment about this patch which I forgot to send
> > > last time:
> > > 
> > > +struct grub_json
> > > +{
> > > +  void *tokens;
> > > +  char *string;
> > > +  grub_size_t idx;
> > > +};
> > > +typedef struct grub_json grub_json_t;
> > > 
> > > 
> > > Why is tokens declared as void* rather than jsmntok_t* ? There are a bunch of
> > > casts back to jsmntok_t * as part of your wrapper.
> > > 
> > > Is the idea to attempt to provide a separation between grub_json_* and jsmn (to
> > > allow other libraries to implement the JSON module?).  Or is there some sort of
> > > pointer arithmetic I'm missing that requires the use of void*?
> > > 
> > > If it's the former, why don't we just do:
> > > 
> > >   typedef jsmntok_t grub_json_tok_t;
> > >   struct grub_json
> > >   {
> > >     grub_json_tok_t *tokens;
> > >     char *string;
> > >     grub_size_t idx;
> > >   };
> > >   typedef struct grub_json grub_json_t;
> > > 
> > > 
> > > For now and worry about other libraries later?
> > 
> > The reason is that we'd have to include "jsmn.h" in
> > "include/grub/json.h" to make `jsmntok_t` available. I definitely
> > want to avoid this in order to not leak any decls from jsmn into
> > users of "json.h" and to be able to have "jsmn.h" live in
> > "grub-core/lib/json". It's also not possible to have a forward
> > declaration here due to how `jsmntok_t` is declared.
> > 
> > Patrick
> 
> 
> In which case, looking again at this patchset - grub_json_t
> is always used as an opaque value by consumers of json.h .
> 
> Why not put a forward declaration of grub_json_t in include/grub/json.h,
> and stick the implementation in grub-core/lib/json.c :
> 
>   include/grub/json.h:
> 
>     typedef struct grub_json_t grub_json_t;
>   
>   grub-core/lib/json.c:
>   ...
>   #include <grub/json.h>
>   #include "jsmn.h"
>   ...
>   struct grub_json_t {
>         jsmntok_t* tokens;
>         ...
>   };
> 
> 
> Unless there is something I'm missing doesn't the above achieve the
> desired result? 

If we'd do that, all calls to `grub_json_get{child,value}` would
have to return a dynamically allocated `grub_json_t` structure as
the storage size of `grub_json_t` would not be known at compile
time and thus cannot be allocated on the caller's stack. Right
now, the caller can just declare the structure on-stack and have
the interface fill up these structures for him, without any need
for dynamic memory allocation.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]