consistent formatting #427

yanovich · 2022-12-02T18:52:11Z

Huge whitespace-only change.

beldmit · 2022-12-02T18:53:29Z

Не-не-не... Автогенерированные файлы про эллиптику лучше не трогать

yanovich · 2022-12-02T19:02:34Z

Скажите какие -- уберу. Может добавить форматирования в генератор?

beldmit · 2022-12-02T19:03:29Z

ecp_id_*

beldmit · 2022-12-02T19:21:58Z

Результаты пока скорее странные.
Чтоб я помнил, откуда взялся файл getopt.h
Часть файлов, кажется, я форматировал openssl-ным скриптом когда-то.

yanovich · 2022-12-02T20:51:56Z

Часть файлов, кажется, я форматировал openssl-ным скриптом когда-то.

Только если очень небольшую часть )

$ list=$(git ls-tree --name-only -r HEAD | grep -v ecp_id_.* | grep '[.][ch]$'); ../util/check-format.pl $list
...
151049 (150032 indentation, 0 '#if' nesting indent, 3 syntax, 96 whitespace, 50 length, 868 other) issues have been found by ../util/check-format.pl

vt-alt · 2022-12-03T03:56:04Z

Странные замены навскидку:

- static const char *gost_envnames[] =
-     { "CRYPT_PARAMS", "GOST_PBE_HMAC", "GOST_PK_FORMAT" };
+ static const char *gost_envnames[] = {
+     "CRYPT_PARAMS", "GOST_PBE_HMAC", "GOST_PK_FORMAT"};

Уже писал в #426 - почему это отформатировалось так а следующее за ним эдак:

- const uint8_t grasshopper_pi_inv[0x100] = {
-         0xA5, 0x2D, 0x32, 0x8F, 0x0E, 0x30, 0x38, 0xC0,    // 00..07
-         0x54, 0xE6, 0x9E, 0x39, 0x55, 0x7E, 0x52, 0x91,    // 08..0F
...
+     0xA5, 0x2D, 0x32, 0x8F, 0x0E, 0x30, 0x38, 0xC0, // 00..07
+     0x54, 0xE6, 0x9E, 0x39, 0x55, 0x7E, 0x52, 0x91, // 08..0F
...
- const uint8_t grasshopper_lvec[16] = {
-         0x94, 0x20, 0x85, 0x10, 0xC2, 0xC0, 0x01, 0xFB,
-         0x01, 0xC0, 0xC2, 0x10, 0x85, 0x20, 0x94, 0x01
- };
+ const uint8_t grasshopper_lvec[16] = {0x94,
+                                       0x20,
+                                       0x85,

Иногда аргументы выровнены по краю открывающей скобки, а иногда нет:

-     return EVP_Cipher(ctx->actx, ctx->km, zero_iv,
-         EVP_CIPHER_key_length(EVP_CIPHER_CTX_cipher(ctx->actx)) +
-         EVP_CIPHER_CTX_block_size(ctx->cctx));
+     return EVP_Cipher(ctx->actx,
+                       ctx->km,
+                       zero_iv,
+                       EVP_CIPHER_key_length(EVP_CIPHER_CTX_cipher(ctx->actx))
+                           + EVP_CIPHER_CTX_block_size(ctx->cctx));
+ }

-     gost2012_hash_block((gost2012_hash_ctx *) EVP_MD_CTX_md_data(ctx), data,
-                         count);
+     gost2012_hash_block(
+         (gost2012_hash_ctx *)EVP_MD_CTX_md_data(ctx), data, count);

-         while (get_line
-                (check_file, inhash, filename, verbose, &expected_hash_size)) {
+         while (get_line(
+             check_file, inhash, filename, verbose, &expected_hash_size)) {

Вернуло одну проверку в предыдущею строку (видимо потому что она влезала в 80 символов) хотя все остальные в столбец:

      if ((ctx = OPENSSL_zalloc(sizeof(*ctx))) != NULL
          && (ctx->proverr_handle = proverr_new_handle(core, in)) != NULL
          && (ctx->libctx = OSSL_LIB_CTX_new()) != NULL
-         && (ctx->e = ENGINE_new()) != NULL
-         && populate_gost_engine(ctx->e)) {
+         && (ctx->e = ENGINE_new()) != NULL && populate_gost_engine(ctx->e)) {
          ctx->core_handle = core;

Не сказать, что это внезапное склеивание добавляет понятности, при этом в некоторых структурах, массивах и вызовах функций (как в примере с EVP_Cipher) иногда решает перенести все даже короткие аргументы в столбец.

yanovich · 2022-12-03T10:26:41Z

Уже писал в #426 - почему это отформатировалось так а следующее за ним эдак:

Тут не знаю, но думаю, что из-за комментариев.

Иногда аргументы выровнены по краю открывающей скобки, а иногда нет:

Это AllowAllArgumentsOnNextLine. Можно отключить.

Вернуло одну проверку в предыдущею строку (видимо потому что она влезала в 80 символов) хотя все остальные в столбец:

Это странное поведение.

Не сказать, что это внезапное склеивание добавляет понятности, при этом в некоторых структурах, массивах и вызовах функций (как в примере с EVP_Cipher) иногда решает перенести все даже короткие аргументы в столбец.

Для аргументов AllowAllArgumentsOnNextLine, по умолчанию true. и BinPackArguments: false. Поэтому или одна строка, или по одному.

yanovich · 2022-12-03T12:38:47Z

До патча:

list=$(git ls-tree --name-only -r HEAD | grep -v ecp_id_.* | grep -v gost_grasshopper_precompiled.c | grep '[.][ch]$'); ../util/check-format.pl $list
...
8898 (4019 indentation, 0 '#if' nesting indent, 3 syntax, 541 whitespace, 390 length, 3945 other) issues have been found by ../util/check-format.pl

С патчем:

list=$(git ls-tree --name-only -r HEAD | grep -v ecp_id_.* | grep -v gost_grasshopper_precompiled.c | grep '[.][ch]$'); ../util/check-format.pl $list
...
3272 (2383 indentation, 0 '#if' nesting indent, 3 syntax, 94 whitespace, 49 length, 743 other) issues have been found by ../util/check-format.pl

Решение не идеальное, но в правильном направлении. Лучше ничего нет, всё равно. Только если руками править все 8898 ошибкок формативания.

benchmark/sign.c

beldmit · 2022-12-04T11:31:36Z

gost_err* генерируются OpenSSL-ным скриптом. Собственно, их лучше перегенерировать, и если их формат не совпадают с форматированием по скрипту, то его оставить.

Я с подозрением смотрю на getopt.h, но пока проще переформатировать, чем убивать.

В остальном меня всё скорее устраивает. @vt-alt?

yanovich · 2022-12-04T11:39:30Z

.git-blame-ignore-revs

@@ -0,0 +1,2 @@
+cf83bf3cb9108370bef19284278152e14acb115a
+390d81c8b7247afe28c4316174d6fba2d617f25b


Это надо будет поменять, когда окончательная редакция первого патча будет

beldmit · 2022-12-04T15:05:59Z

benchmark/sign.c

-    "md_gost12_512", "gost2012_512", "A",
-    "md_gost12_512", "gost2012_512", "B",
-    "md_gost12_512", "gost2012_512", "C",
+    "md_gost12_256",


Вот с этим что-то сделать можно?

нет, насколько я понимаю. это инициализация массива. никаких опций по массивам нет. если в одну строку не влазит, то делает 1 элемент в 1й строке. Для javascript'a есть опция BreakArrays, чтобы принудительно делать 1-в-1, а наоборот -- вообще ничего.

2975996 поправил все массивы и вызовы фунций

beldmit · 2022-12-04T15:06:38Z

gost89.c

-    {0X4, 0XA, 0X9, 0X2, 0XD, 0X8, 0X0, 0XE, 0X6, 0XB, 0X1, 0XC, 0X7, 0XF,
-     0X5, 0X3}
-};
+gost_subst_block GostR3411_94_TestParamSet = {{0X1,


и с этим

аналогично. это инициализация массивов.

beldmit · 2022-12-04T15:08:07Z

gost_asn1.c

+                           ASN1_IMP(
+                               GOST_KEY_TRANSPORT, key_agreement_info,
+                               GOST_KEY_AGREEMENT_INFO,
+                               0)} ASN1_NDEF_SEQUENCE_END(GOST_KEY_TRANSPORT) IMPLEMENT_ASN1_FUNCTIONS(GOST_KEY_TRANSPORT)



И этот файл как-то странно выглядит

Макросы вообще плохо форматируются. Тут только если убрать из списка форматирования и проверки.

Можно ещё /* clang-format off/on */ окружить те места, где не нравится

beldmit · 2022-12-04T15:09:19Z

gost_ec_keyx.c

+                                 8,
+                                 ukm_source + 16,
+                                 8,
+                                 1)


Туда же

Сейчас настройки для аргументов при вызове функции:

или одна строка

или перенос на строку и одна строка

или 1 аргумент на строку.

Тут есть опция, чтобы паковать в допустимую ширину BinPackArguments, можно поменять.

Я делал настройки по правилам OpenSSL. Там в пункте 2:
Don’t put multiple statements, or assignments, on a single line. ... ... The same applies to function headers with a long argument list. ...

beldmit · 2022-12-04T15:09:37Z

gost_ec_keyx.c

+            VKO_compute_key(key,
+                            EC_KEY_get0_public_key(EVP_PKEY_get0(peer_key)),
+                            (EC_KEY *)EVP_PKEY_get0(my_key),
+                            data->shared_ukm,


Зачем три строки вместо одной?

Вопрос не понял. В этом, конкретно, вызове вообще всё осталось как было.

beldmit · 2022-12-04T15:16:49Z

test_ciphers.c

+        printf(cBLUE "# Tests for %s [%s]" cNORM "\n", t->algname, standard);
+        for (inplace = 0; inplace <= 1; inplace++)
+            ret |= test_block(ciph,
+                              t->algname,


Зачем по строчке на аргумент?

Форматирование вызова функции можно перенастроить

beldmit · 2022-12-04T15:17:09Z

test_context.c

-    { SN_magma_ctr, },
-    { SN_id_tc26_cipher_gostr3412_2015_kuznyechik_ctracpkm, 256 / 8 },
-    { 0 },
+    {


Зачем?

Так быть не должно. AlignArrayOfStructures:None установлен.

beldmit · 2022-12-04T15:17:43Z

test_digest.c

 };

 static const char MAC_omac_acpkm1[] = {
-    0xB5,0x36,0x7F,0x47,0xB6,0x2B,0x99,0x5E,0xEB,0x2A,0x64,0x8C,0x58,0x43,0x14,0x5E,
+    0xB5,


Зачем?

Массив. Без поняния, что тут можно сделать.

beldmit · 2022-12-04T15:17:51Z

test_digest.c

 };

 static const char MAC_omac_acpkm2[] = {
-    0xFB,0xB8,0xDC,0xEE,0x45,0xBE,0xA6,0x7C,0x35,0xF5,0x8C,0x57,0x00,0x89,0x8E,0x5D,
+    0xFB,


Зачем?

Массив

beldmit · 2022-12-04T15:18:51Z

test_gost2814789.c

-  /*
+    {/* Calculated by libcapi10, CryptoPro CSP 3.6R2, Mac OSX */
+     16,
+     {0x74,


Зачем?

Массив

beldmit · 2022-12-04T15:20:32Z

Ад какой-то :(
Переформатирование по аргументу на строчку совсем лишнее.

Можем мы это как-то приблизить к openssl-ному формату, или идти постепенно, для начала - с пробелами и табуляциями?

yanovich · 2022-12-04T17:27:48Z

Ад какой-то :( Переформатирование по аргументу на строчку совсем лишнее.

Можно поменять для вызовов функций.

Можем мы это как-то приблизить к openssl-ному формату, или идти постепенно, для начала - с пробелами и табуляциями?

У OpenSSL же нет своего формативания. Только проверка. В linux и node.js тоже используют clang-format. Промышленных альтернтив нет, насколько я понимаю. Было что-то на основе gcc, но не взлетело. Так что варианта 2: или всё руками, или пользоваться тем, что есть.

yanovich · 2022-12-04T20:44:52Z

Попробовал BinPackArguments:true. Массивы стали выглядеть лучше, но ошибок при проверке скриптом от opensslстало больше. Видимо, для некоторых массивов, инициализация востпринимается clang'ом как вызов конструктора. Я только так могу предположить, почему этот параметр влияет на форматирование инициализации массивов.

3362 (2467 indentation, 0 '#if' nesting indent, 3 syntax, 94 whitespace, 53 length, 745 other) issues have been found by ../util/check-format.pl

vt-alt · 2022-12-05T06:48:32Z

Если какой-то скрипт будет заставлять человека менять форматирование на такое какое генерирует clang-fiormat, то как человек должен заранее понять, что в одном месте надо было писать аргументы пока влезут до 80го столбца, в другом вертикально по одному, а в третьем 6й и 7й из них склеить в одну строку, в одном месте надо сделать отступ от скобки, а другом от начала предыдущей строки?

И это без учета, что теряется git blame — за такую потерю в анализе кода, который писали много лет, надо хоть что-то приобрести. А затраты труда на поломать то что есть — минуты?

У OpenSSL же нет своего формативания. Только проверка. В linux и node.js тоже используют clang-format.

В Linux clang-format не енфорсится,, а предлагается как утилита в помощь в форматировании. https://www.kernel.org/doc/html/latest/process/clang-format.html

Так что варианта 2: или всё руками, или пользоваться тем, что есть.

Заменять кривое ручное форматирование, на кривое машинное как-то странно. Если мы теряем git blame, то хотя бы оно должно выглядеть хорошо.

vt-alt · 2022-12-05T07:42:51Z

Только сейчас прочитал про .git-blame-ignore-revs , интересно, на сколько хорошо это работает.

yanovich · 2022-12-05T09:18:52Z

Только сейчас прочитал про .git-blame-ignore-revs , интересно, на сколько хорошо это работает.

новые пустые строчки остаются только. https://github.com/gost-engine/engine/blame/5e777b8b3de7f0eee09054f1ecd9cd0993ee4762/gost_eng.c

yanovich · 2022-12-05T09:33:53Z

Если какой-то скрипт будет заставлять человека менять форматирование на такое какое генерирует clang-fiormat, то как человек должен заранее понять, что в одном месте надо было писать аргументы пока влезут до 80го столбца, в другом вертикально по одному, а в третьем 6й и 7й из них склеить в одну строку, в одном месте надо сделать отступ от скобки, а другом от начала предыдущей строки?

Например, можно просто сделать $ ./.github/format.sh. Но обычно люди ставят интеграцию в свой редактор (см. соответствующий раздел), через некоторое время вызов форматирования становится рефлекторным, и проекты, где им нельзя пользоваться, начинают очущаться какими-то опасными.

У OpenSSL же нет своего формативания. Только проверка. В linux и node.js тоже используют clang-format.

В Linux clang-format не енфорсится,, а предлагается как утилита в помощь в форматировании. https://www.kernel.org/doc/html/latest/process/clang-format.html

Там форсится check-patch.pl. Имеется опыт ручного формативания файла с новым драйвером в до-клангформатное время ).

И линукс, и опенссл проходят аудит кода в разных нерусских трёхбуквенных организациях, и им надо там каждую строчку каждого патча объяснять, поэтому там не будет такого PR, как этот. Но это же не достоинство, а как раз наоборот.

vt-alt · 2022-12-05T13:36:49Z

Там форсится check-patch.pl. Имеется опыт ручного формативания файла с новым драйвером в до-клангформатное время ).

~/linux/torvalds (master)$ grep -ic clang scripts/checkpatch.pl
0

Да и перед постом я грепнул, что clang-format упоминается только в документации (просмотрел её ещё раз) и в коде в строках типа /* clang-format off */.

yanovich · 2022-12-05T14:16:35Z

У OpenSSL же нет своего формативания. Только проверка. В linux и node.js тоже используют clang-format.

В Linux clang-format не енфорсится,, а предлагается как утилита в помощь в форматировании. https://www.kernel.org/doc/html/latest/process/clang-format.html

Я нигде не утверждал, что clang-format в linux "енфорсится". Факт "использования" его в линуксе отрицать бессмысленно. Основная мысль моего комментария, который Вы процитировали: "скриптов на стиль в мире как грязи, а промышленых алтернатив клангформату нет". А нет их потому, что для разработки такой утилиты, чтобы можно было с закрытыми глазами ей пользоваться, надо писать компилятор С, или использовать уже готовый.

consistent formatting

390d81c

yanovich mentioned this pull request Dec 2, 2022

clang_format #426

Open

fixup! consistent formatting

cf83bf3

ignore reformat in blame

16ea890

yanovich added 3 commits December 3, 2022 14:47

fixup! consistent formatting

a8a461e

fixup! consistent formatting

044f359

CI format test

ba47e42

yanovich force-pushed the format branch 2 times, most recently from f35e136 to 99c2e7a Compare December 3, 2022 12:16

fixup! CI format test

5a587fc

yanovich force-pushed the format branch from 99c2e7a to 5a587fc Compare December 3, 2022 12:17

github-advanced-security bot found potential problems Dec 3, 2022

View reviewed changes

benchmark/sign.c Fixed Show fixed Hide fixed

fix CodeQL error

5a4f74d

fixup! consistent formatting

12aeacd

yanovich commented Dec 4, 2022

View reviewed changes

beldmit reviewed Dec 4, 2022

View reviewed changes

clang_format: BinPackArguments

2975996

fixup! ignore reformat in blame

5e777b8

vt-alt force-pushed the master branch from 456e71b to ede3886 Compare March 22, 2024 02:48

		@@ -0,0 +1,2 @@
		cf83bf3cb9108370bef19284278152e14acb115a
		390d81c8b7247afe28c4316174d6fba2d617f25b

consistent formatting #427

Are you sure you want to change the base?

consistent formatting #427

Conversation

yanovich commented Dec 2, 2022

beldmit commented Dec 2, 2022

yanovich commented Dec 2, 2022

beldmit commented Dec 2, 2022

beldmit commented Dec 2, 2022

yanovich commented Dec 2, 2022

vt-alt commented Dec 3, 2022

yanovich commented Dec 3, 2022

yanovich commented Dec 3, 2022

beldmit commented Dec 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beldmit commented Dec 4, 2022

yanovich commented Dec 4, 2022

yanovich commented Dec 4, 2022

vt-alt commented Dec 5, 2022

vt-alt commented Dec 5, 2022

yanovich commented Dec 5, 2022

yanovich commented Dec 5, 2022 • edited Loading

vt-alt commented Dec 5, 2022

yanovich commented Dec 5, 2022

yanovich commented Dec 5, 2022 •

edited

Loading