[tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name

[Kirill Shcherbatov]

Thank you for review!

On 27.06.2018 18:57, n.pettik wrote:
> I see that in this patch you provide changes which are related not
> only to renaming, but to refactoring of some functions, to removing
> #ifdef SQLITE_OMIT_TRIGGER macros etc> Lets mention these facts in commit message.
Changed Trigger structure name to sql trigger to avoid
confusing similarity with Tarantool trigger structure.
Refactored related code to match tarantool codestyle.

> Moreover, if you placed this patch before ’sql: move Triggers to server ‘,
> you would avoid additional diffs in alter.cc and trigger.c
I believe that it is not critical.
First of all, I've changed triggers logic a lot, so some functions was dropped. The would be 
another useless diff on refactoring.
Then, It is not trivial to rebase this changes to be before 'move trigger comment' as 
I've reworked sqlite3{Begin, End}Trigger a lot and I don't like rebase constructive patch changes.
This, as usual, cause new bugs and typos. Let's do not mix important and not really changes.
This is just only refactoring patch and it doesn't really matter, where is it located.
 
> You can fit ’table’ and ‘onconf’ args in one line.
-                                           table,
-                                           onconf);
+                                           table, onconf);

> I would also fix comments inside struct definition.
 struct sql_trigger {
-       char *zName;            /* The name of the trigger                        */
+       /** The name of the trigger. */
+       char *zName;
        /** The ID of space the trigger refers to. */
        uint32_t space_id;
-       u8 op;                  /* One of TK_DELETE, TK_UPDATE, TK_INSERT         */
-       u8 tr_tm;               /* One of TRIGGER_BEFORE, TRIGGER_AFTER */
-       Expr *pWhen;            /* The WHEN clause of the expression (may be NULL) */
-       IdList *pColumns;       /* If this is an UPDATE OF <column-list> trigger,
-                                  the <column-list> is stored here */
-       TriggerStep *step_list; /* Link list of trigger program steps             */
+       /** One of TK_DELETE, TK_UPDATE, TK_INSERT. */
+       u8 op;
+       /** One of TRIGGER_BEFORE, TRIGGER_AFTER. */
+       u8 tr_tm;
+       /** The WHEN clause of the expression (may be NULL). */
+       Expr *pWhen;
+       /**
+        * If this is an UPDATE OF <column-list> trigger,
+        * the <column-list> is stored here
+        */
+       IdList *pColumns;
+       /** Link list of trigger program steps. */
+       TriggerStep *step_list;
        /** Next trigger associated with the table. */
        struct sql_trigger *next;
 };

> Place type of return value at separate line. Also add names of args to prototype.
-void sql_trigger_finish(Parse *, TriggerStep *, Token *);
+void
+sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
+                  struct Token *token);

> You don’t need to declare all variables beforehand: its obsolete style
> used in SQLite.
It is important here, as there is goto clause before first usage. 
This variable should be initialized with NULL. 

> Put ‘or’ operator at previous line:
-       if (sqlite3FixTriggerStep(&fixdb, trigger->step_list)
-           || sqlite3FixExpr(&fixdb, trigger->pWhen))
+       if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
+           sqlite3FixExpr(&fixdb, trigger->pWhen))

> 
> 	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
> 	    sqlite3FixExpr(&fixdb, trigger->pWhen))
> 
>> -/*
>> +/**
>>  * Create and populate a new TriggerPrg object with a sub-program
>>  * implementing trigger pTrigger with ON CONFLICT policy orconf.
>> + *
>> + * @param parser Current parse context.
>> + * @param trigger sql_trigger to code.
>> + * @param table trigger is attached to.
>> + * @param orconf ON CONFLICT policy to code trigger program with.
>> + *
>> + * @retval not NULL on success.
>> + * @retval NULL on error.
>>  */
>> static TriggerPrg *
>> -codeRowTrigger(Parse * pParse,	/* Current parse context */
>> -	       Trigger * pTrigger,	/* Trigger to code */
>> -	       Table * pTab,	/* The table pTrigger is attached to */
>> -	       int orconf	/* ON CONFLICT policy to code trigger program with */
>> -    )
>> +sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
>> +			struct Table *table, int orconf)
> 
> Lets call it vdbe_code_row_trigger() or smth like that.
I already have another one function vdbe_code_row_trigger(and also vdbe_code_row_trigger_direct)
that generates vdbe code.
As I see, sql_row_trigger and sql_row_trigger_program aimed to return TriggerPrg * pointer and MAY cause
generating VDBE code. Let's keep it as is.

> Why VM is temporary?
moped is not mine
this comment almost was there, I've just moved it; believe, this VM lives on stack af this function

> We call bool variables with ‘is_’ or ‘if_’  prefix, as a rule.
-       bool recursive = (trigger->zName && !(user_session->sql_flags &
-                                             SQLITE_RecTriggers));
+       bool is_recursive = (trigger->zName && !(user_session->sql_flags &
+                                                SQLITE_RecTriggers));

> We call bool variables with ‘is_’ or ‘if_’ prefixes, as a rule.
It is not boolean value.. It used as array index. For me, boolean array index is a little more
confusing.