Conversation
DimaTrushin
left a comment
DimaTrushin
left a comment
There was a problem hiding this comment.
Обрати внимание, что я не повторял замечания. Если одна и та же ошибка всречалась несколько раз, я комментировал лишь единожды. Потому я ожидаю, что ты поправишь все аналогичные замечания всюду.
Набросал несколько замечаний. Там есть идейные длинные рассуждения с объяснением архитектуры и каких-то возможных решений. Постарайся прочитать их все прежде чем что-то менять.
| set(CMAKE_CXX_STANDARD 20) | ||
|
|
||
| add_library(distsysenv | ||
| src/utils/utils.h |
There was a problem hiding this comment.
Не надо добавлять h файлы в add_executable или add_library. Они не дают единицу трансляции, они нужны только для подключения в cpp файлы. А вот cpp файлы дают единицы трансляции.
| *.dwo | ||
|
|
||
| build/ | ||
| .clang-format No newline at end of file |
There was a problem hiding this comment.
новая строка в том числе в конце gitignore
| cmake_minimum_required(VERSION 3.14) | ||
| project(DistSysEnv LANGUAGES CXX) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 20) |
There was a problem hiding this comment.
Добавь строчку
set(CMAKE_CXX_STANDARD_REQUIRED True)А то cmake разрешено откатиться на более низкий стандарт.
Или более новая версия
target_compile_features(target_name PRIVATE cxx_std_20)| using TOffset = int64_t; | ||
| using TTimerName = std::string; | ||
|
|
||
| using SimulationClock = float; |
There was a problem hiding this comment.
Глобально лучше вместо сырого using-га использовать простенький класс. В особенности для работы со временем. Плюс время может быть разным. И у него могут быть единицы измерения. Тут я так понимаю абстрактное время, и сырой тип вроде должен работать.
| using Bytes = std::vector<Byte>; | ||
|
|
||
| using TOffset = int64_t; | ||
| using TTimerName = std::string; |
There was a problem hiding this comment.
Плохой using. Это супер специальный using явно относящийся к таймеру. Я бы еще понял, если бы у тебя была категория просто Name или Description или что-то в этом духе. А тут прям под конкретный класс. Это скорее всего означает, что этому using-гу тут не место, его место рядом с классом для таймера. А во-вторых, не ясно зачем для имени таймера вводить псевдоним для типа. Если ты хочешь не перепутать строки, например у тебя конструктор вида
Timer t("123", "abc");и тебе надо пояснить смысл этих строк, то ты делаешь strong type aliases
template<class T, class Tag>
class Alias {
public:
explicit Alias(const T& value) : value_(value) {}
explicit Alias(T&& value) : value_(std::move(value)) {}
operator T() const& noexcept {
return value_;
}
operator T() && noexcept {
return std::move(value_);
}
private:
T value_;
};
using Name = Alias<std::string, struct name_tag>;
using Description = Alias<std::string, struct description_tag>;И потом можешь писать
Timer t(Name{"123"}, Description{"abc"});Но мне что-то подсказывает, что этому не место тут.
| std::map<TKey, TVal> storage_; | ||
| }; | ||
|
|
||
| } // namespace distsysenv No newline at end of file |
There was a problem hiding this comment.
Добавь пустую строку в конце каждого файла. Дело в том, что гит на это ругается и пользователь твоего репозитория будет видеть сообщения об ошибках от гита. Не делай так.
Добавить в clang-format правило:
InsertNewlineAtEOF: true
| return it->second; | ||
| } | ||
|
|
||
| enum class Status { Ok, Error }; |
There was a problem hiding this comment.
не объявляй типы в случайных местах. Их фиг найдешь таким образом. Это вообще выглядит как супер общий тип для глобальных назначений. Ну либо его надо в начале этого класса в публичной части объявить.
|
|
||
| namespace distsysenv { | ||
|
|
||
| struct EchoNode { |
There was a problem hiding this comment.
Если что-то пределяешь или объявляешь в cpp файле, то обязательно дополнительно заворачивай в anonymous namespace!
| NetworkSettings net_settings{ | ||
| .drop_prob = 0.0f, .min_delay = 1, .max_delay = 5}; | ||
| Network network(net_settings, 42); | ||
| manager.RegisterNetwork(std::move(network)); |
There was a problem hiding this comment.
Плохое название. Имя говорит, что ты регистрируешь объект, а std::move говорит, что ты объект поглощаешь. И вот не ясно, то ли ты этот объект внутрь переместил, то ли название метода не верное.
| manager.Schedule( | ||
| Event::MessageSend(0, a, b, Message::FromDescription("hello", {}))); |
There was a problem hiding this comment.
Желательно чтобы соединения между узлами можно было в коде тоже как-то отразить. Сейчас у тебя нет никакой топологии сети. У тебя просто каждый с каждым соединен как я понимаю (или точнее у тебя все с сетью соединены и еще какие-то там случайные соединения с логгером).
2 participants
No description provided.