Fixed unity build errors with gcc-12 #2886

2022-07-01T00:11:42Z

biodranik commented

2022-07-01 00:11:42 +00:00

(Migrated from github.com)

Never use global using namespace std;

vng (Migrated from github.com) reviewed 2022-07-01 03:22:38 +00:00

editor/changeset_wrapper.cpp

					
				@ -41,3 +34,3 @@

				string const static kVowels = "aeiouy";

				std::string_view constexpr kVowels = "aeiouy";

vng (Migrated from github.com) commented

2022-07-01 03:13:07 +00:00

Put this definitions under namespace {} ?

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-07-01 03:10:06 +00:00

strings will be created each function call from initializer_list<char const *>

Make std::string kMainTags[] = {} above.

strings will be created each function call from initializer_list<char const *> Make std::string kMainTags[] = {} above.

generator/cities_boundaries_builder.cpp

vng (Migrated from github.com) commented

2022-07-01 03:15:26 +00:00

remove tabs

vng (Migrated from github.com) reviewed 2022-07-01 03:29:42 +00:00

generator/generator_tool/generator_tool.cpp

vng (Migrated from github.com) commented

2022-07-01 03:28:45 +00:00

We already have using namespace generator

biodranik (Migrated from github.com) reviewed 2022-08-04 23:06:02 +00:00

generator/cities_boundaries_builder.cpp

biodranik (Migrated from github.com) commented

2022-08-04 23:06:01 +00:00

?

biodranik (Migrated from github.com) reviewed 2022-08-04 23:06:12 +00:00

editor/changeset_wrapper.cpp

					
				@ -41,3 +34,3 @@

				string const static kVowels = "aeiouy";

				std::string_view constexpr kVowels = "aeiouy";

biodranik (Migrated from github.com) commented

2022-08-04 23:06:12 +00:00

уже внутри

biodranik commented

2022-08-05 06:38:36 +00:00

(Migrated from github.com)

PTAL. Там ещё много где можно править, но пока хватит.

vng (Migrated from github.com) reviewed 2022-08-05 09:11:42 +00:00

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 08:54:48 +00:00

Неужели так красивее чем было?

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 08:56:09 +00:00

Чем отличие от предидущего? Еще и static?

editor/editor_tests/new_feature_categories_test.cpp

					
				@ -21,4 +18,2 @@

				  auto const & disabled = CategoriesHolder::kDisabledLanguages;

				  for (auto const & locale : CategoriesHolder::kLocaleMapping)

				  {

vng (Migrated from github.com) commented

2022-08-05 08:59:57 +00:00

у нас есть более лаконичные функции типа base::IsExist(disabled, lang)

editor/editor_tests/new_feature_categories_test.cpp

vng (Migrated from github.com) commented

2022-08-05 09:00:48 +00:00

А sort скомпилировался без std?

generator/cities_boundaries_builder.cpp

vng (Migrated from github.com) commented

2022-08-05 09:02:00 +00:00

мы пишем внутри namespace без отступа обычно

geometry/region2d/binary_operators.cpp

vng (Migrated from github.com) commented

2022-08-05 09:06:30 +00:00

пусть бы уже сидело вначале чем 4 раза копировать :)

indexer/classificator.cpp

vng (Migrated from github.com) commented

2022-08-05 09:08:41 +00:00

тут вообще лучше цикл написать

for (auto & obj : m_objs)
  obj.Sort();

тут вообще лучше цикл написать ``` for (auto & obj : m_objs) obj.Sort(); ```

indexer/classificator.cpp

vng (Migrated from github.com) commented

2022-08-05 09:09:03 +00:00

{-1, -1}

indexer/data_header.cpp

vng (Migrated from github.com) commented

2022-08-05 09:10:27 +00:00

Раз уж пошли сюда, то тут лучше написать case MapType::Country: return {worldH + 1, high};
А внизу UNRECHABLE_CODE();

Раз уж пошли сюда, то тут лучше написать case MapType::Country: return {worldH + 1, high}; А внизу UNRECHABLE_CODE();

biodranik (Migrated from github.com) reviewed 2022-08-05 10:26:37 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 10:26:37 +00:00

Это clang-tidy посоветовал обойтись без ненужной аллокации памяти, я тут с ним согласен :)

biodranik (Migrated from github.com) reviewed 2022-08-05 10:33:18 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 10:33:18 +00:00

Предыдущий генератор всегда выдавал одно и то же, т.е. не был случайным.
Статик, чтобы не создавать много копий устройства, есть баги и креши в gcc, с этим связанные. Но это вообще лучше системно сделать, чтобы был класс-враппер над девайсом, и он при запуске сидил все генераторы. Ладно, статик уберу, тут и без него будет норм (кстати, частое вытягивание сида из random_device может быть очень тормозным, поэтому лучше реюзать engines).

Это всё clang tidy в цлион

1. Предыдущий генератор всегда выдавал одно и то же, т.е. не был случайным. 2. Статик, чтобы не создавать много копий устройства, есть баги и креши в gcc, с этим связанные. Но это вообще лучше системно сделать, чтобы был класс-враппер над девайсом, и он при запуске сидил все генераторы. Ладно, статик уберу, тут и без него будет норм (кстати, частое вытягивание сида из random_device может быть очень [тормозным](https://stackoverflow.com/questions/39288595/why-not-just-use-random-device), поэтому лучше реюзать engines). Это всё clang tidy в цлион

biodranik (Migrated from github.com) reviewed 2022-08-05 10:39:20 +00:00

editor/editor_tests/new_feature_categories_test.cpp

biodranik (Migrated from github.com) commented

2022-08-05 10:39:20 +00:00

Да, где-то ещё остался юзинг получается. Но странно, что без юнити собрался.

biodranik (Migrated from github.com) reviewed 2022-08-05 10:45:48 +00:00

geometry/region2d/binary_operators.cpp

biodranik (Migrated from github.com) commented

2022-08-05 10:45:48 +00:00

Нет, это как раз самый плохой лик неймспейса был. Важно оставить только внутри скоупа, а не протягивать его везде во все исходники unity.

biodranik commented

2022-08-05 11:12:12 +00:00

(Migrated from github.com)

PTAL

vng (Migrated from github.com) reviewed 2022-08-05 13:29:37 +00:00

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 13:29:37 +00:00

Это некрасиво и нормальный stl s1 + s2 + s3 делает лучше (должен) s1.append(s2).append(s3) без лишних аллокаций.

Верни назад, а то и const убирается и контекст начинает модифицировать переменные.

Это некрасиво и нормальный stl s1 + s2 + s3 делает лучше (должен) s1.append(s2).append(s3) без лишних аллокаций. Верни назад, а то и const убирается и контекст начинает модифицировать переменные.

vng (Migrated from github.com) reviewed 2022-08-05 13:32:55 +00:00

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 13:32:55 +00:00

Пусть дает одинаковые .. Тут вообще эвристика какая-то

biodranik (Migrated from github.com) reviewed 2022-08-05 15:01:57 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 15:01:57 +00:00

Так const специально убран для RVO, по совету clang-tidy.

Почему stl должен делать лучше? Явно же создаются две временные переменные, в худшем случае с двумя ненужными аллокациями, и всегда с двумя ненужными копированиями.

Так const специально убран для RVO, по совету clang-tidy. Почему stl должен делать лучше? Явно же создаются две временные переменные, в худшем случае с двумя ненужными аллокациями, и всегда с двумя ненужными копированиями.

biodranik (Migrated from github.com) reviewed 2022-08-05 15:03:54 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 15:03:54 +00:00

Попытка сматчить фичу. Пускай разные точки будут на разных вызовах, больше шансов сматчить.

vng (Migrated from github.com) reviewed 2022-08-05 16:07:57 +00:00

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 16:02:18 +00:00

так это возникло когда ты начал мутить с append. В s1 + s2 + s3 временный объект, мувнется в результат
давай считать: value.append(" ").append(key) - 2 аллокации (ок, в каком-то случаем может быть одна). s1 + s2 + s3 тоже 2 аллокации. А в умном stl есть мегатехники оптимизации operator+

- так это возникло когда ты начал мутить с append. В s1 + s2 + s3 временный объект, мувнется в результат - давай считать: value.append(" ").append(key) - 2 аллокации (ок, в каком-то случаем может быть одна). s1 + s2 + s3 тоже 2 аллокации. А в умном stl есть мегатехники оптимизации operator+

vng (Migrated from github.com) commented

2022-08-05 16:02:55 +00:00

Ты корявишь код там где это абсолютно не надо, а в итоге еще и вредно.

indexer/classificator.hpp

vng (Migrated from github.com) commented

2022-08-05 13:45:27 +00:00

зачем?

biodranik (Migrated from github.com) reviewed 2022-08-05 18:22:24 +00:00

indexer/classificator.hpp

biodranik (Migrated from github.com) commented

2022-08-05 18:22:24 +00:00

Базовый класс и наследник специально создавались, чтобы не оверрайдить свои одинаковые функции? Может тогда не надо базовый класс?

biodranik (Migrated from github.com) reviewed 2022-08-05 18:29:48 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 18:29:47 +00:00

Посмотри внимательно, есть в if один else, где value возвращается по RVO.

Генерируемого кода меньше с append, можешь сам убедиться: https://godbolt.org/z/bexedef3M

Посмотри внимательно, есть в if один else, где value возвращается по RVO. Генерируемого кода меньше с append, можешь сам убедиться: https://godbolt.org/z/bexedef3M

vng (Migrated from github.com) reviewed 2022-08-05 18:33:49 +00:00

indexer/classificator.hpp

vng (Migrated from github.com) commented

2022-08-05 18:33:49 +00:00

тут просто наследование кода стратегий, это не используется полиморфно

vng (Migrated from github.com) reviewed 2022-08-05 18:42:45 +00:00

editor/changeset_wrapper.cpp

vng (Migrated from github.com) commented

2022-08-05 18:42:45 +00:00

ok, const не нужен
даже если там его и меньше, это то место, где читаемость лучше странных спичек, которых еще может и не быть

- ok, const не нужен - даже если там его и меньше, это то место, где читаемость лучше странных спичек, которых еще может и не быть

biodranik (Migrated from github.com) reviewed 2022-08-05 19:12:25 +00:00

editor/changeset_wrapper.cpp

biodranik (Migrated from github.com) commented

2022-08-05 19:12:25 +00:00

Все компиляторы показывают разницу, на арм64 151 vs 169 https://godbolt.org/z/Y5c7xcYhq
Я против того, чтобы жертвовать быстродействием и батареей в угоду достаточно спорной читабельности одной строчки кода.

Но чтобы удовлетворить ревьюера и наконец-то исправить компиляцию с gcc12, отменю это изменение.

Все компиляторы показывают разницу, на арм64 151 vs 169 https://godbolt.org/z/Y5c7xcYhq Я против того, чтобы жертвовать быстродействием и батареей в угоду достаточно спорной читабельности одной строчки кода. Но чтобы удовлетворить ревьюера и наконец-то исправить компиляцию с gcc12, отменю это изменение.