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

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, Nov 08, 2019 at 01:30:28PM +0000, Max Tottenham wrote:
> On 11/07, Patrick Steinhardt wrote:
> > On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> > > On 11/06, Patrick Steinhardt wrote:
> > > > 
> > > > 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
> 
> Ah, good point. I've been thinking about this for a while and I think
> dealing with casts from void* really is the best we can do.
> 
> Perhaps jsmn upstream would accept a patch that does:
> 
>   - typedef struct {
>   + typedef struct jsmntok_t {
>     ...
>     } jsmntok_t;
> 
> If they do we could revisit this later.

I've created a pull request upstream to change exactly that --
let's wait and see. In the meantime we'll have to use the current
`void *` workaround, but I'll make sure to update this as soon as
the PR got merged (if ever).

Patrick

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