Re: [review][json] My review

From: Gavin Lambert (boost_at_[hidden])
Date: 2020-09-30 01:27:06

On 23/09/2020 13:36, I wrote:
> On 23/09/2020 12:52, Vinnie Falco wrote:
>> The interfaces are built up in layers. At the lowest layer you have
>> basic_parser which concerns itself only with processing the input
>> buffer and turning it into semantic components. It does _not_ have the
>> responsibility of buffering input to present it as a larger component,
>> that's the job of the handler. basic_parser is exposed to users so
>> they can use their own schemes for dealing with the semantic
>> components.
>>
>> Above basic_parser, you have parser which uses value_stack. The
>> value_stack does take responsibility for buffering input, and
>> producing DOM values. As with basic_parser, both of these components
>> are exposed to users so they can write their own layers on top.
> 
> My point is that this seems like the wrong choice of layers.  As it 
> stands, consumers should never care about the basic_parser layer, and 
> instead will actually want a different layer (not currently exposed) 
> that is between the two, which exposes complete values that are not in 
> any DOM.
> 
> The current form of basic_parser could still exist, if you like, but it 
> should be made an implementation detail and the complete-value 
> basic_parser actually exposed to consumers.

To restate this in a clearer way: it is still my belief that aspects of 
json::parser performance considerations have "polluted" the design of 
json::basic_parser to the point where it is not actually a meaningful 
interface.

As such I strongly recommend that json::basic_parser be turned into an 
implementation detail and not part of the public-facing part of the 
library.  The library should only be used via its DOM interface 
(json::parser/json::value), as that is the only one that is actually 
properly baked.

I don't regard this as a reason to change my vote to accept the library 
(and it's too late anyway), as the DOM interface by itself is highly 
useful.  I just think that the basic_parser interface should be treated 
as out of scope.