On Thu, Mar 26, 2020 at 10:35:03PM +0100, Fabien COELHO wrote:
> Your point is to store the gset/aset status into the meta field, even if the
> command type is SQL. This is not done for gset, which relies on the non-null
> prefix, and breaks the assumption that meta is set to something only when
> the command is a meta command. Why not. I updated the comment, so now meta
> is none/gset/aset when command type is sql, and I removed the aset field.
Yes, that's the point I was trying to make. Thanks for sending a new
version.
> - * meta The type of meta-command, or META_NONE if command is SQL
> + * meta The type of meta-command, if command is SQL META_NONE,
> + * META_GSET or META_ASET which dictate what to do with the
> + * SQL query result.
I did not quite get why you need to update this comment. The same
concepts as before apply.
>> + /* 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.
>
> Added (I think, if I understood what you suggested.).
+ /* coldly skip empty result under \aset */
+ else if (meta == META_ASET && ntuples <= 0)
+ break;
Yes, that's what I meant. Now it happens that we don't have a
regression test to cover the path where we have no tuples. Could it
be possible to add one?
+ Assert((meta == META_NONE && varprefix == NULL) ||
+ ((meta == META_GSET || meta == META_ASET) && varprefix != NULL));
+
Good addition. That would blow up if meta is set to something else
than {META_NONE,META_GSET,META_ASET}, so anybody changing this code
path will need to question if he/she needs to do something here.
Except for the addition of a test case to skip empty results when
\aset is used, I think that we are pretty good here.
--
Michael
