From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 76B482260B for ; Wed, 27 Jun 2018 12:35:51 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id whcNVQmTLmTm for ; Wed, 27 Jun 2018 12:35:51 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A3C6E21146 for ; Wed, 27 Jun 2018 12:35:50 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name References: <81986d1f9307191bd3d3e37514dc32fadb7e6970.1530029141.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <3563ea71-30c6-3f20-8484-7970786c3de6@tarantool.org> Date: Wed, 27 Jun 2018 19:35:48 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: "n.pettik@corp.mail.ru" 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 trigger, - the 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 trigger, + * the 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.