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
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
