[tarantool-patches] Re: [PATCH] sql: remove struct Table
i.koptelov
ivan.koptelov at tarantool.org
Tue Feb 12 16:55:14 MSK 2019
> On 11 Feb 2019, at 20:58, n.pettik <korablev at tarantool.org> wrote:
>
>
>> On 6 Feb 2019, at 20:17, i.koptelov <ivan.koptelov at tarantool.org> wrote:
>>
>>>> On 1 Feb 2019, at 14:05, Ivan Koptelov<ivan.koptelov at tarantool.org> wrote:
>>>>
>>>> Thank you for the review! Sorry for all these code style errors.
>>>> I consider review changes minor, so I put diff b/w first and
>>>> second version of patch at the end of the letter.
>>>> (second commit on the branch 'review fixes' would be squashed)
>>> Don’t do it next time, pls. Instead, inline fixes as an answers to comments,
>>> especially when it comes for huge diff ( 209 insertions(+), 259 deletions(-))
>>> Otherwise, it takes a while to track fixed chunks of code in a whole diff.
>> Sorry for this. All review fixes below is inlined.
>
> Well, thank you. But don’t put fixes in a separate commit, at least when
> you push your branch to the remote repository. You may *accidentally*
> fail during final commit squashing and no-one will notice that.
>
> Travis status was entirely negative: It can’t be compiled.
> You forgot to add forward declaration of space_def.
> I fixed that and a couple of minor issues more. Here is a diff
> (I’ve squashed your commits and pushed on top of your branch).
>
> Please, check it out. If it is OK, squash them and then patch LGTM.
Thank you for the fixes! Ok, I would not put my fixes in separate commit in future.
I checked out the diff, it’s ok for me except one small issue. I’ve marked
it below.
>
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 874fd682f..1352f6efe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2759,6 +2760,9 @@ struct Parse {
> TriggerPrg *pTriggerPrg; /* Linked list of coded triggers */
> With *pWith; /* Current WITH clause, or NULL */
> With *pWithToFree; /* Free this WITH object at the end of the parse */
> + /** Space triggers are being coded for. */
> + struct space *triggered_space;
> + /** A space being constructed by CREATE TABLE */
Are you sure that this comment (last line) should be here? Is it about all lines below?
> /**
> * Number of FK constraints declared within
> * CREATE TABLE statement.
More information about the Tarantool-patches
mailing list