Fixed routing bug. #326

rtsisyk · 2021-04-22T16:40:38Z

vng commented

2021-04-22 16:40:38 +00:00

(Migrated from github.com)

organicmaps/organicmaps#323

https://git.omaps.dev/organicmaps/organicmaps/issues/323

rtsisyk reviewed 2021-04-22 16:40:38 +00:00

biodranik (Migrated from github.com) approved these changes 2021-04-23 10:58:11 +00:00

biodranik (Migrated from github.com) left a comment

LGTM with comments

base/base_tests/buffer_vector_test.cpp

					
				@ -20,0 +25,4 @@

				  buffer_vector<ElementT, 2> v;

				  v.append(2, element);

				  v.push_back(v[0]);

biodranik (Migrated from github.com) commented

2021-04-23 10:52:37 +00:00

А есть такой же тест на emplace?

base/buffer_vector.hpp

					
				@ -147,3 +119,4 @@

				        m_size = newSize;

				        return;

				      }

				      else

biodranik (Migrated from github.com) commented

2021-04-23 10:54:28 +00:00

Может делать это внутри SwitchToDynamic, передавая размер?

base/buffer_vector.hpp

					
				@ -447,3 +395,3 @@

				private:

				  void SwitchToDynamic()

				  void SwitchToDynamic(size_t toReserve)

biodranik (Migrated from github.com) commented

2021-04-23 10:55:38 +00:00

Сигнатура не как у вектора, будут же много где копирования ненужные. Может обе оставить? Или добавить пушбэк с &&?

routing/index_graph_starter_joints.hpp

biodranik (Migrated from github.com) commented

2021-04-23 10:57:29 +00:00

Так в чём был баг?

vng (Migrated from github.com) reviewed 2021-04-23 15:02:03 +00:00

base/base_tests/buffer_vector_test.cpp

					
				@ -20,0 +25,4 @@

				  buffer_vector<ElementT, 2> v;

				  v.append(2, element);

				  v.push_back(v[0]);

vng (Migrated from github.com) commented

2021-04-23 15:02:02 +00:00

Ох, emplace не используется в таком контексте (ну или не надо использовать). Если ее так предусматривать, надо извращаться.

vng (Migrated from github.com) reviewed 2021-04-23 15:02:28 +00:00

base/buffer_vector.hpp

					
				@ -147,3 +119,4 @@

				        m_size = newSize;

				        return;

				      }

				      else

vng (Migrated from github.com) commented

2021-04-23 15:02:28 +00:00

Можно, да

vng (Migrated from github.com) reviewed 2021-04-23 15:04:55 +00:00

base/buffer_vector.hpp

					
				@ -447,3 +395,3 @@

				private:

				  void SwitchToDynamic()

				  void SwitchToDynamic(size_t toReserve)

vng (Migrated from github.com) commented

2021-04-23 15:04:55 +00:00

std::vector push_back как раз таки внутри создает копию. Тут я сделал явно - это и есть фикс ошибки. Лучше иметь одну функцию (T t), чтобы можно было либо push_back(t), либо push_back(std::move(t)).

vng (Migrated from github.com) reviewed 2021-04-23 15:06:04 +00:00

routing/index_graph_starter_joints.hpp

vng (Migrated from github.com) commented

2021-04-23 15:06:04 +00:00

Когда вектор внутри переаллоцируется, parentWeights[i] протухает. Надо принимать по значению или создавать копию внутри функции (первое лучше).

vng (Migrated from github.com) reviewed 2021-04-23 15:08:56 +00:00

base/buffer_vector.hpp

					
				@ -447,3 +395,3 @@

				private:

				  void SwitchToDynamic()

				  void SwitchToDynamic(size_t toReserve)

vng (Migrated from github.com) commented

2021-04-23 15:08:56 +00:00

Решил не усложнять и всегда принимать по значению - на самом деле экономии никакой не будет от 2-х сигнатур.

biodranik (Migrated from github.com) reviewed 2021-04-23 15:38:44 +00:00

routing/index_graph_starter_joints.hpp

biodranik (Migrated from github.com) commented

2021-04-23 15:38:44 +00:00

А обязательно ради этого перекраивать интерфейс buffer_vector? Он же тут ни при чём, надо было просто тут сделать push_back(ElementType(parentWeights[i])). Или я не понимаю бенефит?

biodranik (Migrated from github.com) reviewed 2021-04-23 15:39:04 +00:00

routing/index_graph_starter_joints.hpp

biodranik (Migrated from github.com) commented

2021-04-23 15:39:04 +00:00

Да ещё и коммент написать про буффервектор )

Dushistov commented

2021-04-23 15:41:49 +00:00

(Migrated from github.com)

What about usage of boost::container::small_vector instead of hand made container?

What about usage of `boost::container::small_vector` instead of hand made container?

biodranik (Migrated from github.com) reviewed 2021-04-23 15:42:58 +00:00

base/buffer_vector.hpp

					
				@ -447,3 +395,3 @@

				private:

				  void SwitchToDynamic()

				  void SwitchToDynamic(size_t toReserve)

biodranik (Migrated from github.com) commented

2021-04-23 15:42:58 +00:00

Лучше иметь одну функцию (T t), чтобы можно было либо push_back(t), либо push_back(std::move(t)).

buffer_vector::push_back(T t) создаст первую копию, а если вызовется m_vec.push_back(T const & t) вместо (T &&) то и вторая копия будет. Если хочешь эффективно, то внутри надо явно std::move версию push_back всегда вызывать, вместо обычной копирующей.

> Лучше иметь одну функцию (T t), чтобы можно было либо push_back(t), либо push_back(std::move(t)). buffer_vector::push_back(T t) создаст первую копию, а если вызовется m_vec.push_back(T const & t) вместо (T &&) то и вторая копия будет. Если хочешь эффективно, то внутри надо явно std::move версию push_back всегда вызывать, вместо обычной копирующей.

biodranik (Migrated from github.com) reviewed 2021-04-23 15:44:13 +00:00

base/base_tests/buffer_vector_test.cpp

					
				@ -20,0 +25,4 @@

				  buffer_vector<ElementT, 2> v;

				  v.append(2, element);

				  v.push_back(v[0]);

biodranik (Migrated from github.com) commented

2021-04-23 15:44:13 +00:00

Я бы критически просмотрел ещё раз имеющиеся тесты buffer_vector, чтобы всякие странные кейзы точно были покрыты, а то мало ли.

vng commented

2021-04-23 15:45:56 +00:00

(Migrated from github.com)

Good point about boost small_vector.

make small incremental changes
colleagues are not always happy about boost in core

Will consider to replace it in future.

Good point about boost small_vector. - make small incremental changes - colleagues are not always happy about boost in core Will consider to replace it in future.

vng (Migrated from github.com) reviewed 2021-04-23 16:02:50 +00:00

base/buffer_vector.hpp

					
				@ -447,3 +395,3 @@

				private:

				  void SwitchToDynamic()

				  void SwitchToDynamic(size_t toReserve)

vng (Migrated from github.com) commented

2021-04-23 16:02:49 +00:00

так и делается, в любом случае будет одна копия

vng (Migrated from github.com) reviewed 2021-04-23 16:04:48 +00:00

routing/index_graph_starter_joints.hpp

vng (Migrated from github.com) commented

2021-04-23 16:04:48 +00:00

Сделал безопаснее. Так или иначе, поведение std::vector такое же - он делает копию перед переаллокацией. Я написал выше что принимая (T t) мы ничего не теряем.