Re: [Tarantool-patches] [PATCH 5/5] Apply Clang formatter

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Vladislav Shpilevoy <v.shpilevoy@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