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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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-цикл. Он не должен как тело функции форматироваться.

Далее я смотрел только бегло, и там еще дохрена таких же
бесполезных изменений.