[Tarantool-patches] [PATCH 5/5] Apply Clang formatter
Kirill Yukhin
kyukhin at tarantool.org
Mon Nov 2 10:35:03 MSK 2020
Hello,
On 31 окт 00:04, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> I will repeat here what I said in the chat.
>
> На русском, чтоб понятнее получилось.
>
> Тикет был создан, когда команда состояла из стажеров более, чем
> на половину. Большая часть из них засылала патчи как будто сдавали
> задачки в универе за час до дедлайна. Без стиля кода вообще. Тогда
> у патчей было комментариев по 50 штук, большая часть которых -
> проблемы со стилем.
>
> Но во-первых, это были далеко не все проблемы. Огромное количество
> замечаний было по мелким багам, отсутствию тестов или их бесполезности,
> по проблемам с английским, когда текст комментариев был нечитаем, и тд.
> Потому даже если бы этот патч появился тогда, теперь я понимаю, что
> он бы помог не сильно.
>
> Во-вторых, по поводу аргумента, что это отнимает время ревьюера. Каждый
> коммент бросить в письмо отнимает пару секунд, буквально. Авто-форс
> стиля не сэкономит времени нисколько. 0 минут на патч он сэкономит. Даже
> для самого ужасного патча.
>
> Сейчас качество патчей возросло значительно. По мере роста
> опыта стажеров, и когда ушли самые заядлые нарушители. Потому кол-во
> стилистических проблем измеряется единицами.
>
> В-третьих, этот патч на форматирование вносит большое количество весьма
> и весьма спорных изменений. После которых объективно код читать сложнее,
> физически. Смотри далее несколько комментов по самым дебильным местам.
> Если ты этим надеешься исправить наши редкие споры про стиль некоторых
> выражений, то это не поможет. Теперь мы будем спорить, где надо воткнуть
> уродливый коммент для игнора форматтера, чтобы он не сделал код еще хуже.
>
> В-четвертых, это говно придется влить во все ветки, так как иначе будет
> невозможно патчи пропушивать вниз по версиям. Так что это испортит
> историю еще сиьнее.
>
> В-пятых, это не будет единичное изменение истории. Некоторые
> места были сделаны с выравниванием для упрощения изменения и чтения.
> Форматтер все эти места херит. См далее комменты. 9 штук.
>
> Подпункт пятого замечания - что будешь делать, когда снова прибежит
> какой-нибудь любитель плюсов, и скажет, что надо сделать длину строки 120
> символов? Или если решим еще какую-нибудь хрень поменять вроде пробелов
> приведениях типов? Снова будет патч на косари строк, чтоб все это
> обновить?
>
> Суммируя, патч очень плох, и не делает лучше вообще ничего. Я не понимаю,
> зачем это льется.
I won't discuss any of the points, since they all were discussed multiple
times and we all agreed to give it a try.
Answers inlined.
> > 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к изменений, но
> это не значит, что можно их не смотреть.
I've removed them.
> > #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. Это пиздец как неудобно.
I've increased penalty for breaking on assignement and now this looks much
better.
> > @@ -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. Что здесь улучшилось? Это же жесть какая-то. Ты смотрел вообще этот патч?
This is a fix for breaking the rule of 80 cols. But this hunk reveals significant
flaw in current formatter (even in master branch): it tries to keep function
with its arguements on the same line and only after that done it checks everything
else. It'd be greate to be able to set penalty (low) for breaking on param list
and make the change like this:
- 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)
Which reads better for me.
> > @@ -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. Новый вариант выглядит хуже, сложнее читать.
I am not sure. Anyway I think this is not a blocker for having fully automated
code formatting.
> > @@ -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. В какой вселенной новая версия лучше? Как должен быть устроен глаз,
> чтобы вот это воспринимать было проще чем то, что было до?
Same as for 4.
> > 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, то форматтер перехуярит весь
> енум, похерив историю снова.
This is just yet another place which should be accompanied with
/* clang format [off|on] */. Done.
> > + _(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. Похожий коммент уже был выше, но это просто апогей. После
> такого кода мне иначе придется очки скоро покупать.
Formatter want's to align all param lines with first param.
I am not sure, I can fix it.
> > 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, но походу это говно применяется даже к
> обычным выражениям.
Unfortunatelly, formatter doesn't care about type of statements.
What is even worse, is that its knobs doesn't allow to discriminate
between logical and non-logical operators.
I can make it nice:
return strnindex(coll_icu_case_first_strs + 1, str, len,
- coll_icu_case_first_MAX - 1) + 1;
+ coll_icu_case_first_MAX - 1) +
+ 1;
BUT. The patch will increase by 20% since our practice it to
put logical operators at the end of line, and all those places
will be `fixed` by formatter. E.g.:
- if (priv_def_create_from_tuple(&priv, tuple) != 0 ||
- grant_or_revoke(&priv) != 0)
+ if (priv_def_create_from_tuple(&priv, tuple) != 0
+ || grant_or_revoke(&priv) != 0)
> > @@ -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-цикл. Он не должен как тело функции форматироваться.
>
> Далее я смотрел только бегло, и там еще дохрена таких же
> бесполезных изменений.
Fixed.
After all, having p.3 and p.8 unresolved I guess we cannot proceed
without changes to formatter.
--
Regards, Kirill Yukhin
More information about the Tarantool-patches
mailing list