On Fri, Nov 29, 2019 at 10:34:05AM +0100, Fabien COELHO wrote:
>> It seems to me that there is no point to have the variable aset in
>> Command because this structure includes already MetaCommand, so the
>> information is duplicated. [...] Perhaps I am missing something?
>
> Yep. ISTM that you are missing that aset is not an independent meta command
> like most others but really changes the state of the previous SQL command,
> so that it needs to be stored into that with some additional fields. This is
> the same with "gset" which is tagged by a non-null "varprefix".
>
> So I cannot remove the "aset" field.
Still sounds strange to me to invent a new variable to this structure
if it is possible to track the exact same thing with an existing part
of a Command, or it would make sense to split Command into two
different structures with an extra structure used after the parsing
for clarity?
>> And I would suggest to change readCommandResponse() to use a MetaCommand
>> in argument.
>
> MetaCommand is not enough: we need varprefix, and then distinguishing
> between aset and gset. Although this last point can be done with a
> MetaCommand, ISTM that a bool is_aset is clear and good enough. It is
> possible to switch if you insist on it, but I do not think it is desirable.
>
> Attached v4 removes an unwanted rebased comment duplication and does minor
> changes while re-reading the code.
Well, it still looks cleaner to me to just assign the meta field
properly within ParseScript(), and you get the same result. And it is
also possible to use "meta" to do more sanity checks around META_GSET
for some code paths. So I'd actually find the addition of a new
argument using a meta command within readCommandResponse() cleaner.
- * varprefix SQL commands terminated with \gset have this set
+ * varprefix SQL commands terminated with \gset or \aset have this set
Nit from v4: varprefix can be used for \gset and \aset, and the
comment was not updated.
+ /* coldly skip empty result under \aset */
+ if (ntuples <= 0)
+ break;
Shouldn't this check after \aset? And it seems to me that this code
path is not taken, so a test would be nice.
- } while (res);
+ } while (res != NULL);
Useless diff.
The conflicts came from the switch to the common logging of
src/common/. That was no big deal to solve.
--
Michael