[Tarantool-patches] [PATCH 5/5] Apply Clang formatter
Konstantin Osipov
kostja.osipov at gmail.com
Mon Nov 2 00:40:30 MSK 2020
* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [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 <new> /* for placement new */
> > +#include <new> /* for placement new */
> > #include <stdio.h> /* snprintf() */
> > #include <ctype.h>
> > #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
More information about the Tarantool-patches
mailing list