#41 Replace TMaybe to std::optional after update #81

antonporodnikov · 2024-03-05T00:09:24Z

Новая попытка PR после апдэйта:

Заменил TMaybe на std::optional
Удалил все файлы связанные с TMaybe
Почистил зависимости: инклюды, мэйк-файлы и т. п.

GitSparTV

Привет. Поздравляю с первым PR! 🙂

Из всех файлов отметил только одно критичное замечание (⚠️).
В остальных местах всё достаточно одинаково — поменять названия, использовать аналогичный метод.
Местами неконсистентный стиль кода после рефакторинга, думаю, автор ишью посмотрит и скажет как сделать.

GitSparTV · 2024-03-05T18:15:31Z

client/impl/ydb_internal/grpc_connections/grpc_connections.cpp

-        discoveryMode ? discoveryMode.GetRef() : DefaultDiscoveryMode_,
-        sslCredentials ? sslCredentials.GetRef() : SslCredentials_,
-        credentialsProviderFactory ? credentialsProviderFactory.GetRef() : DefaultCredentialsProviderFactory_);
+        database.has_value() ? database.value() : DefaultDatabase_,


Обычно предпочитают явное поведение, тут оно есть.
Но я бы уточнил у автора ишью о стиле. В оригинале используется operator bool.

Ещё есть вопрос поводу самой проверки. value() внутри тоже проверяет has_value() и в случае отсутствия выбрасывает исключение. Если мы и так до этого проверили has_value(), зачем снова это делать?

У нас тут есть default значение ещё, я бы использовал value_or

Исправлено.

GitSparTV · 2024-03-05T19:17:57Z

client/ydb_datastreams/datastreams.cpp

-                            } else if (settings.RetentionPeriodHours_.Defined()) {
-                                req.set_retention_period_hours(*settings.RetentionPeriodHours_.Get());
+                            if (settings.RetentionStorageMegabytes_.has_value()) {
+                                req.set_retention_storage_megabytes(settings.RetentionStorageMegabytes_.value());


Здесь тоже двойная проверка, но в оригинале её не было.

Глянь: https://godbolt.org/z/sGccs6Eea

lea rdi, [rbp - 4] // Взять o call O<int>::Get() const // Вызвать над ним Get mov esi, dword ptr [rax] // Дереференс указателя

Исправлено.

GitSparTV · 2024-03-05T19:22:23Z

client/ydb_federated_topic/impl/federated_write_session.cpp

@@ -67,7 +67,7 @@ void TFederatedWriteSession::OpenSubSessionImpl(std::shared_ptr<TDbInfo> db) {
        .ReadyToAcceptHander([self = shared_from_this()](NTopic::TWriteSessionEvent::TReadyToAcceptEvent& ev){
            TDeferredWrite deferred(self->Subsession);
            with_lock(self->Lock) {
-                Y_ABORT_UNLESS(self->PendingToken.Empty());
+                Y_ABORT_UNLESS(!(self->PendingToken.has_value()));


Скобочки лишние как по мне

Убрал, но по-идее они есть ещё в коде. В спешке почему-то подумалось, что такой явный стиль (если это можно так назвать) будет уместным. Исправлено.

Upd. Постарался убрать по всему своему коду.

GitSparTV · 2024-03-05T19:37:10Z

client/ydb_query/tx.h

@@ -4,6 +4,8 @@

 #include <client/ydb_types/fluent_settings_helpers.h>

+#include <util/stream/output.h>


Почему здесь output понадобился?

error: unknown type name 'IOutputStream' на 41 строке. В свою очередь output.h был включен в maybe.h (возможно вложенно, через ещё один заголовочный файл). Я убрал включение maybe.h и добавил напрямую output.h соответственно в tx.h

О как. Спасибо, что пояснил

GitSparTV · 2024-03-05T19:39:05Z

client/ydb_table/impl/table_client.cpp

@@ -1135,19 +1135,19 @@ void TTableClient::TImpl::SetQuery(const TDataQuery& queryData, Ydb::Table::Quer
 void TTableClient::TImpl::SetQueryCachePolicy(const std::string&, const TExecDataQuerySettings& settings,
    Ydb::Table::QueryCachePolicy* queryCachePolicy)
 {
-    queryCachePolicy->set_keep_in_cache(settings.KeepInQueryCache_ ? settings.KeepInQueryCache_.GetRef() : false);
+    queryCachePolicy->set_keep_in_cache(settings.KeepInQueryCache_ ? settings.KeepInQueryCache_.value() : false);


Теоретически может стать value_or

Исправил.

GitSparTV · 2024-03-05T19:40:24Z

client/ydb_table/table.cpp

        }
        for (const auto& familyPolicy : policy.ColumnFamilies_) {
            auto* familyProto = proto->mutable_storage_policy()->add_column_families();
            if (familyPolicy.Name_) {
-                familyProto->set_name(familyPolicy.Name_.GetRef());
+


⚠️ А куда строчка делась?

Непростительный фэйл так сказать, из-за спешки к сожалению допустил подобное. Исправлено.

GitSparTV · 2024-03-05T19:44:05Z

client/ydb_table/table.cpp

-    if (QueryStats_.Defined()) {
-        return NYdb::TProtoAccessor::GetProto(*QueryStats_.Get()).query_plan();
+    if (QueryStats_.has_value()) {
+        return NYdb::TProtoAccessor::GetProto(*QueryStats_).query_plan();


GitSparTV · 2024-03-05T19:51:24Z

client/ydb_types/s3_settings.h

@@ -2,6 +2,8 @@

 #include "fluent_settings_helpers.h"

+#include <string>


Откуда пришёл?

Не знаю как правильно здесь поступить, ошибки error: use of undeclared identifier 'std' при компиляции в строка 18, 20 - 22. Решил включением string.

Скорее всего тоже транзитивно

GitSparTV · 2024-03-05T19:56:12Z

library/cpp/getopt/small/last_getopt_opts.cpp

@@ -6,6 +6,7 @@
 #include <library/cpp/colorizer/colors.h>
 #include <library/cpp/string_utils/misc/misc.h>

+#include <util/generic/algorithm.h>


С main ветки пришло?

Нет, я добавил из-за error: use of undeclared identifier 'IsIn' в строках 111, 120, 129, 138. Опять же algorithm.h до этого был неявно включён через maybe.h, который я убрал.

GitSparTV · 2024-03-05T20:15:08Z

library/cpp/testing/gtest_extensions/ut/gtest_extensions_ut.cpp

-    EXPECT_EQ(GtestPrint(TMaybe<std::string>("hello world")), "\"hello world\"");
-    EXPECT_EQ(GtestPrint(TMaybe<std::string>()), "nothing");
-    EXPECT_EQ(GtestPrint(Nothing()), "nothing");
+    EXPECT_EQ(GtestPrint(std::optional<std::string>("hello world")), "\"hello world\"");


А остался ли смысл в этом тесте, если optional часть STL? TMaybe был классом библиотеки, теперь нет

Убрал.

Действительно, этот тест можно убрать

antonporodnikov · 2024-03-06T12:25:00Z

@GitSparTV Спасибо за ревью. Сделал все исправления, откомментировал. Постарался убрать по всему своему коду подобное написание !(*.has_value()), вроде всё почистил.

GitSparTV · 2024-03-06T13:10:16Z

client/ydb_persqueue_core/impl/write_session_impl.cpp

Поменяй на value_or

Исправил.

GitSparTV

Как говорится — LGTM :)
Всё исправлено и готово к просмотру заказчиком

client/ydb_persqueue_core/impl/write_session_impl.h

client/ydb_persqueue_core/ut/read_session_ut.cpp

library/cpp/getopt/small/modchooser.cpp

library/cpp/iterator/concatenate.h

antonporodnikov · 2024-03-06T14:12:24Z

@Gazizonoki Спасибо за ревью. Исправил замечания.

antonporodnikov added 2 commits March 4, 2024 19:41

replacement: ydb_scripting.*; build=succeed

792cce2

replacement: *.*; build=succeed

b8055f5

antonporodnikov requested a review from GitSparTV March 5, 2024 00:09

antonporodnikov added the student-projects label Mar 5, 2024

antonporodnikov linked an issue Mar 5, 2024 that may be closed by this pull request

Replace TMaybe to std::optional #41

Closed

antonporodnikov self-assigned this Mar 5, 2024

delete maybe files, clear dependencies; build=succeed

eb0c555

GitSparTV suggested changes Mar 6, 2024

View reviewed changes

antonporodnikov requested a review from GitSparTV March 6, 2024 13:08

GitSparTV reviewed Mar 6, 2024

View reviewed changes

GitSparTV approved these changes Mar 6, 2024

View reviewed changes

corrections after first review by GitSparTV

d8a3dea

antonporodnikov requested a review from Gazizonoki March 6, 2024 13:33

ydb_topic/impl/write_session_impl.cpp fix

c2a8e80

Gazizonoki requested changes Mar 6, 2024

View reviewed changes

antonporodnikov requested a review from Gazizonoki March 6, 2024 14:11

Gazizonoki approved these changes Mar 6, 2024

View reviewed changes

Gazizonoki merged commit 72c6b28 into main Mar 6, 2024
1 check passed

Gazizonoki deleted the 41-TMaybe-to-STLoptional branch March 6, 2024 14:14

antonporodnikov added 2 commits March 6, 2024 17:15

write_session_impl.* fix

9cca962

corrections after Gazizonoki review

84f76f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#41 Replace TMaybe to std::optional after update #81

#41 Replace TMaybe to std::optional after update #81

antonporodnikov commented Mar 5, 2024 •

edited

Loading

GitSparTV left a comment

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

GitSparTV Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

GitSparTV Mar 5, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

GitSparTV Mar 6, 2024

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

GitSparTV Mar 5, 2024

antonporodnikov Mar 6, 2024

Gazizonoki Mar 6, 2024

antonporodnikov commented Mar 6, 2024

GitSparTV Mar 6, 2024

antonporodnikov Mar 6, 2024 •

edited

Loading

GitSparTV left a comment

antonporodnikov commented Mar 6, 2024

		@@ -4,6 +4,8 @@

		#include <client/ydb_types/fluent_settings_helpers.h>

		#include <util/stream/output.h>

		@@ -2,6 +2,8 @@

		#include "fluent_settings_helpers.h"

		#include <string>

#41 Replace TMaybe to std::optional after update #81

#41 Replace TMaybe to std::optional after update #81

Conversation

antonporodnikov commented Mar 5, 2024 • edited Loading

GitSparTV left a 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

antonporodnikov Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonporodnikov Mar 6, 2024 • edited Loading

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

antonporodnikov Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonporodnikov Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonporodnikov Mar 6, 2024 • edited Loading

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

antonporodnikov commented Mar 6, 2024

Choose a reason for hiding this comment

antonporodnikov Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

GitSparTV left a comment

Choose a reason for hiding this comment

antonporodnikov commented Mar 6, 2024

antonporodnikov commented Mar 5, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading

antonporodnikov Mar 6, 2024 •

edited

Loading