[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Minor optimization in json.c
From: |
Eli Zaretskii |
Subject: |
Re: Minor optimization in json.c |
Date: |
Sat, 13 Oct 2018 10:25:22 +0300 |
> From: Philipp Stephani <address@hidden>
> Date: Wed, 3 Oct 2018 21:57:24 +0200
> Cc: address@hidden
>
> Thanks, just 2 stylistic comments.
Thanks for reviewing the code.
> 1. Could you try to eliminate global mutable state (i.e. the
> json_inserted_bytes global variable)? I know that
> with the current threading implementation access is properly synchronized,
> but the global variable is still a
> code smell and unnecessarily couples details of the threading implementation
> with the JSON adapter code. I
> think you should be able to move it into the json_insert_data or
> json_buffer_and_size structures.
I did that, but it needed adding new members to 2 data structures, and
copying the values back and forth between them. Not sure this is more
elegant.
> 2. Could you try factoring out the buffer management code into new functions
> in insdel.c with a simple
> interface? The details on how to keep track of the buffer insertion status
> aren't really related to JSON, and it
> would be better to keep json.c focused on providing the JSON adapter (single
> responsibility principle). Maybe
> with an interface such as:
> void begin_insert_utf8_string (void);
> void insert_utf8_string (const char *chars, ptrdiff_t nbytes);
> void finish_insert_utf8_string (ptrdiff_t nbytes);
I didn't do this. I don't like breaking code into functions that have
no meaning on their own, especially when there's only one caller. (We
do something similar in insert-file-contents, but the structure of the
code and the details are sufficiently different that I decided not to
make a single implementation that would support both.)
I also don't agree with the argument about tracking the insertion
status not being related to JSON: we do similar stuff in many places,
and even json.c itself has similarly "unrelated" code, like the one
under CONSP (lisp) in lisp_to_json_toplevel_1.
The changes are now pushed to master.