From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f169.google.com (mail-lj1-f169.google.com [209.85.208.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6594B469719 for ; Mon, 2 Nov 2020 00:40:33 +0300 (MSK) Received: by mail-lj1-f169.google.com with SMTP id m16so12863026ljo.6 for ; Sun, 01 Nov 2020 13:40:33 -0800 (PST) Date: Mon, 2 Nov 2020 00:40:30 +0300 From: Konstantin Osipov Message-ID: <20201101214030.GB252426@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 5/5] Apply Clang formatter List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org * Vladislav Shpilevoy [20/10/31 12:00]: Автоматический форматтер нужен. Нужно оставить в прошлом уже эту историю с обсуждением стиля патчей - это просто не стоит того чтобы тратить на это время. Так работает экосистема сегодня многих новых языков, и это экономит время. В python есть black, в go есть go fmt. Можно начать с того чтобы предложить людям способ подключить автоформатирование к редактору и добиться того что все новые патчи соответствуют автоформату. > Hi! Thanks for the patch! > > I will repeat here what I said in the chat. > > На русском, чтоб понятнее получилось. > > Тикет был создан, когда команда состояла из стажеров более, чем > на половину. Большая часть из них засылала патчи как будто сдавали > задачки в универе за час до дедлайна. Без стиля кода вообще. Тогда > у патчей было комментариев по 50 штук, большая часть которых - > проблемы со стилем. > > Но во-первых, это были далеко не все проблемы. Огромное количество > замечаний было по мелким багам, отсутствию тестов или их бесполезности, > по проблемам с английским, когда текст комментариев был нечитаем, и тд. > Потому даже если бы этот патч появился тогда, теперь я понимаю, что > он бы помог не сильно. > > Во-вторых, по поводу аргумента, что это отнимает время ревьюера. Каждый > коммент бросить в письмо отнимает пару секунд, буквально. Авто-форс > стиля не сэкономит времени нисколько. 0 минут на патч он сэкономит. Даже > для самого ужасного патча. > > Сейчас качество патчей возросло значительно. По мере роста > опыта стажеров, и когда ушли самые заядлые нарушители. Потому кол-во > стилистических проблем измеряется единицами. > > В-третьих, этот патч на форматирование вносит большое количество весьма > и весьма спорных изменений. После которых объективно код читать сложнее, > физически. Смотри далее несколько комментов по самым дебильным местам. > Если ты этим надеешься исправить наши редкие споры про стиль некоторых > выражений, то это не поможет. Теперь мы будем спорить, где надо воткнуть > уродливый коммент для игнора форматтера, чтобы он не сделал код еще хуже. > > В-четвертых, это говно придется влить во все ветки, так как иначе будет > невозможно патчи пропушивать вниз по версиям. Так что это испортит > историю еще сиьнее. > > В-пятых, это не будет единичное изменение истории. Некоторые > места были сделаны с выравниванием для упрощения изменения и чтения. > Форматтер все эти места херит. См далее комменты. 9 штук. > > Подпункт пятого замечания - что будешь делать, когда снова прибежит > какой-нибудь любитель плюсов, и скажет, что надо сделать длину строки 120 > символов? Или если решим еще какую-нибудь хрень поменять вроде пробелов > приведениях типов? Снова будет патч на косари строк, чтоб все это > обновить? > > Суммируя, патч очень плох, и не делает лучше вообще ничего. Я не понимаю, > зачем это льется. > > > diff --git a/src/box/alter.cc b/src/box/alter.cc > > index 08957f6..c06ec2f 100644 > > --- a/src/box/alter.cc > > +++ b/src/box/alter.cc > > @@ -45,12 +45,12 @@ > > #include "fiber.h" /* for gc_pool */ > > #include "scoped_guard.h" > > #include "third_party/base64.h" > > -#include /* for placement new */ > > +#include /* for placement new */ > > #include /* snprintf() */ > > #include > > #include "replication.h" /* for replica_set_id() */ > > -#include "session.h" /* to fetch the current user. */ > > -#include "vclock.h" /* VCLOCK_MAX */ > > +#include "session.h" /* to fetch the current user. */ > > +#include "vclock.h" /* VCLOCK_MAX */ > > 1. VCLOCK_MAX не используется здесь. Все эти комменты у заголовков бесполезны и > даже иногда вредны. В 99% случаев они вообще устарели, и к реальности отношения > не имеют. Просто удали это, пока эти строки меняешь. Патч может и 6к изменений, но > это не значит, что можно их не смотреть. > > > #include "xrow.h" > > #include "iproto_constants.h" > > #include "identifier.h" > > @@ -68,8 +68,8 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid, > > struct credentials *cr = effective_user(); > > user_access_t has_access = cr->universal_access; > > > > - user_access_t access = ((PRIV_U | (user_access_t) priv_type) & > > - ~has_access); > > + user_access_t access = > > + ((PRIV_U | (user_access_t)priv_type) & ~has_access); > > 2. И что здесь в лучшую сторону изменилось? Что до, что после, выглядит одинаково. И оба > варианта вполне по стилю нашему. Но только теперь надо как-то понимать, что первое не > подходит, иначе будет ебошить CI. Это пиздец как неудобно. > > > bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN; > > if (access == 0) > > return 0; /* Access granted. */ > > @@ -3822,8 +3843,10 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event) > > int > > priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple) > > { > > - if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != 0 || > > - tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != 0) > > + if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != > > + 0 || > > + tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != > > + 0) > > 3. Что здесь улучшилось? Это же жесть какая-то. Ты смотрел вообще этот патч? > > > return -1; > > @@ -4939,17 +4966,16 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) > > if (new_trigger == NULL) > > return -1; > > > > - auto new_trigger_guard = make_scoped_guard([=] { > > - sql_trigger_delete(sql_get(), new_trigger); > > - }); > > + auto new_trigger_guard = make_scoped_guard( > > + [=] { sql_trigger_delete(sql_get(), new_trigger); }); > > 4. Новый вариант выглядит хуже, сложнее читать. > > > @@ -5870,68 +5904,58 @@ on_replace_dd_func_index(struct trigger *trigger, void *event) > > return 0; > > } > > > > -struct trigger alter_space_on_replace_space = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_space, NULL, NULL > > -}; > > +struct trigger alter_space_on_replace_space = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_space, NULL, > > + NULL }; > > 5. В какой вселенной новая версия лучше? Как должен быть устроен глаз, > чтобы вот это воспринимать было проще чем то, что было до? > > > -struct trigger alter_space_on_replace_index = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_index, NULL, NULL > > -}; > > +struct trigger alter_space_on_replace_index = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_index, NULL, > > + NULL }; > > > > -struct trigger on_replace_truncate = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_truncate, NULL, NULL > > -}; > > +struct trigger on_replace_truncate = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_truncate, NULL, NULL }; > > > > -struct trigger on_replace_schema = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_schema, NULL, NULL > > -}; > > +struct trigger on_replace_schema = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_schema, NULL, NULL }; > > > > -struct trigger on_replace_user = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_user, NULL, NULL > > -}; > > +struct trigger on_replace_user = { RLIST_LINK_INITIALIZER, on_replace_dd_user, > > + NULL, NULL }; > > > > -struct trigger on_replace_func = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_func, NULL, NULL > > -}; > > +struct trigger on_replace_func = { RLIST_LINK_INITIALIZER, on_replace_dd_func, > > + NULL, NULL }; > > > > -struct trigger on_replace_collation = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_collation, NULL, NULL > > -}; > > +struct trigger on_replace_collation = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_collation, NULL, NULL }; > > > > -struct trigger on_replace_priv = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_priv, NULL, NULL > > -}; > > +struct trigger on_replace_priv = { RLIST_LINK_INITIALIZER, on_replace_dd_priv, > > + NULL, NULL }; > > > > -struct trigger on_replace_cluster = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_cluster, NULL, NULL > > -}; > > +struct trigger on_replace_cluster = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_cluster, NULL, NULL }; > > > > -struct trigger on_replace_sequence = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_sequence, NULL, NULL > > -}; > > +struct trigger on_replace_sequence = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_sequence, NULL, NULL }; > > > > -struct trigger on_replace_sequence_data = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_sequence_data, NULL, NULL > > -}; > > +struct trigger on_replace_sequence_data = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_sequence_data, NULL, > > + NULL }; > > > > -struct trigger on_replace_space_sequence = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_space_sequence, NULL, NULL > > -}; > > +struct trigger on_replace_space_sequence = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_space_sequence, NULL, > > + NULL }; > > > > -struct trigger on_replace_trigger = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_trigger, NULL, NULL > > -}; > > +struct trigger on_replace_trigger = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_trigger, NULL, NULL }; > > > > -struct trigger on_replace_fk_constraint = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_fk_constraint, NULL, NULL > > -}; > > +struct trigger on_replace_fk_constraint = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_fk_constraint, NULL, > > + NULL }; > > > > -struct trigger on_replace_ck_constraint = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_ck_constraint, NULL, NULL > > -}; > > +struct trigger on_replace_ck_constraint = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_ck_constraint, NULL, > > + NULL }; > > > > -struct trigger on_replace_func_index = { > > - RLIST_LINK_INITIALIZER, on_replace_dd_func_index, NULL, NULL > > -}; > > +struct trigger on_replace_func_index = { RLIST_LINK_INITIALIZER, > > + on_replace_dd_func_index, NULL, NULL }; > > > > /* vim: set foldmethod=marker */ > > diff --git a/src/box/applier.h b/src/box/applier.h > > index 15ca1fc..d519cee 100644 > > --- a/src/box/applier.h > > +++ b/src/box/applier.h > > @@ -47,24 +47,24 @@ > > > > enum { APPLIER_SOURCE_MAXLEN = 1024 }; /* enough to fit URI with passwords */ > > > > -#define applier_STATE(_) \ > > - _(APPLIER_OFF, 0) \ > > - _(APPLIER_CONNECT, 1) \ > > - _(APPLIER_CONNECTED, 2) \ > > - _(APPLIER_AUTH, 3) \ > > - _(APPLIER_READY, 4) \ > > - _(APPLIER_INITIAL_JOIN, 5) \ > > - _(APPLIER_FINAL_JOIN, 6) \ > > - _(APPLIER_JOINED, 7) \ > > - _(APPLIER_SYNC, 8) \ > > - _(APPLIER_FOLLOW, 9) \ > > - _(APPLIER_STOPPED, 10) \ > > - _(APPLIER_DISCONNECTED, 11) \ > > - _(APPLIER_LOADING, 12) \ > > - _(APPLIER_FETCH_SNAPSHOT, 13) \ > > - _(APPLIER_FETCHED_SNAPSHOT, 14) \ > > - _(APPLIER_REGISTER, 15) \ > > - _(APPLIER_REGISTERED, 16) \ > > +#define applier_STATE(_) \ > > + _(APPLIER_OFF, 0) \ > > + _(APPLIER_CONNECT, 1) \ > > + _(APPLIER_CONNECTED, 2) \ > > + _(APPLIER_AUTH, 3) \ > > + _(APPLIER_READY, 4) \ > > + _(APPLIER_INITIAL_JOIN, 5) \ > > + _(APPLIER_FINAL_JOIN, 6) \ > > + _(APPLIER_JOINED, 7) \ > > + _(APPLIER_SYNC, 8) \ > > + _(APPLIER_FOLLOW, 9) \ > > + _(APPLIER_STOPPED, 10) \ > > + _(APPLIER_DISCONNECTED, 11) \ > > + _(APPLIER_LOADING, 12) \ > > + _(APPLIER_FETCH_SNAPSHOT, 13) \ > > + _(APPLIER_FETCHED_SNAPSHOT, 14) \ > > + _(APPLIER_REGISTER, 15) \ > > 6. Из этого изменения становится очевидно, что вся эта ересь > не ограничится одной порчей истории гита. Это будет повторяться. > > Предыдущая версия была специально выровнена с запасом, чтобы можно > было добавлять новые имена, и знать, что они не будут вылезать за > существующие имена. Теперь же если добавить имя на один символ > длиннее, чем APPLIER_FETCHED_SNAPSHOT, то форматтер перехуярит весь > енум, похерив историю снова. > > > + _(APPLIER_REGISTERED, 16) > > > > @@ -159,7 +159,10 @@ box_process_call(struct call_request *request, struct port *port) > > if (func != NULL) { > > rc = func_call(func, &args, port); > > } else if ((rc = access_check_universe_object(PRIV_X | PRIV_U, > > - SC_FUNCTION, tt_cstr(name, name_len))) == 0) { > > + SC_FUNCTION, > > + tt_cstr(name, > > + name_len))) == > > + 0) { > > 7. Похожий коммент уже был выше, но это просто апогей. После > такого кода мне иначе придется очки скоро покупать. > > > diff --git a/src/box/coll_id_def.c b/src/box/coll_id_def.c > > index 9fe0cda..d518ead 100644 > > --- a/src/box/coll_id_def.c > > +++ b/src/box/coll_id_def.c > > @@ -35,35 +35,40 @@ static int64_t > > icu_on_off_from_str(const char *str, uint32_t len) > > { > > return strnindex(coll_icu_on_off_strs + 1, str, len, > > - coll_icu_on_off_MAX - 1) + 1; > > + coll_icu_on_off_MAX - 1) + > > + 1; > > } > > > > static int64_t > > icu_alternate_handling_from_str(const char *str, uint32_t len) > > { > > return strnindex(coll_icu_alternate_handling_strs + 1, str, len, > > - coll_icu_alternate_handling_MAX - 1) + 1; > > + coll_icu_alternate_handling_MAX - 1) + > > + 1; > > } > > > > static int64_t > > icu_case_first_from_str(const char *str, uint32_t len) > > { > > return strnindex(coll_icu_case_first_strs + 1, str, len, > > - coll_icu_case_first_MAX - 1) + 1; > > + coll_icu_case_first_MAX - 1) + > > + 1; > > 8. Выше это было внутри if, но походу это говно применяется даже к > обычным выражениям. > > > @@ -75,15 +77,15 @@ void > > engine_switch_to_ro(void) > > { > > struct engine *engine; > > - engine_foreach(engine) > > - engine->vtab->switch_to_ro(engine); > > + engine_foreach(engine) engine->vtab->switch_to_ro(engine); > > } > > > > int > > engine_bootstrap(void) > > { > > struct engine *engine; > > - engine_foreach(engine) { > > + engine_foreach(engine) > > + { > > 9. Это for-цикл. Он не должен как тело функции форматироваться. > > Далее я смотрел только бегло, и там еще дохрена таких же > бесполезных изменений. -- Konstantin Osipov, Moscow, Russia