Conversation
DimaTrushin
left a comment
DimaTrushin
left a comment
There was a problem hiding this comment.
Обрати внимание, что я делал комментарии почти всегда только разово. Это значит, что во всех похожих ситуациях ты должен сделать изменения по аналогии.
Я непонял зачем ты в какой-то момент переключился с функций на классы. Особенно учитывая, что ты просто вынес из функций явные зависимости в данных в "глобальные переменные", то есть в переменные класса. Если ты хочешь накапливать какую-то информацию между вызовами, то ОК. Но тогда не понятно зачем там в конструкторе передается число и хранится внутри. Это все очень странно и похоже на то, что ты не понимаешь что ты хочешь.
Еще обрати внимание, что встроенные типы надо передавать by value, а все остальное by const reference. Учти это в перегрузке сових шаблонов, а то передавать длинку так будет накладно.
И еще, где система сборки? Где тестирующий код? Как вообще проверить, что твой код запускается?
|
Я тут сделал новый реквест, очень много поправил, пока не исправил то что мы говорили про вот тяжелые типы, что надо внутри по ссылке и ляляля, успею еще, просто хотел чтобы хоть что-то было оценено, а то ведь в прошлый раз я даже смэйк листы не запушил |
DimaTrushin
left a comment
DimaTrushin
left a comment
There was a problem hiding this comment.
Накидал мелких замечаний.
| project(Large-Prime-Numbers-Indenbaum) | ||
|
|
||
| set(CMAKE_CXX_STANDARD 17) | ||
|
|
There was a problem hiding this comment.
Добавь строчку
set(CMAKE_CXX_STANDARD_REQUIRED True)А то cmake разрешено откатиться на более низкий стандарт.
| @@ -0,0 +1,16 @@ | |||
| #include "Testing.h" | |||
|
|
|||
| void RunTests() { | |||
There was a problem hiding this comment.
Не пиши в глобальное пространство имен
| #include "testQS.h" | ||
| #include "Timer.h" | ||
|
|
||
| void RunTests(); No newline at end of file |
There was a problem hiding this comment.
Добавь пустую строку в конце каждого файла. Дело в том, что гит на это ругается и пользователь твоего репозитория будет видеть сообщения об ошибках от гита. Не делай так.
Добавить в clang-format правило:
InsertNewlineAtEOF: true
| @@ -0,0 +1,26 @@ | |||
| #include "testP1.h" | |||
| namespace { | |||
| boost::multiprecision::cpp_int z1 = 450473; | |||
There was a problem hiding this comment.
Заведи юзинги для таких классов тут и не пиши длинные типы. Это же читать не возможно, глаза сломаешь.
| } | ||
| return false; | ||
| } | ||
| bool TestProb() { |
There was a problem hiding this comment.
Ты не консистентен, ты не добавляешь пробелы тут между функциями в приватной зоне, хотя есть пробелы в публичной. Сомневаюсь, что это clang-format так настроен. Проверь, что ты настроил свою ide так, чтобы clang-format применялся насохранение файлов.
| constexpr int MaxCycles = 1000; | ||
| constexpr int ProductSize = 10; | ||
| constexpr int RandomRange = 1000; | ||
| template <class T> |
There was a problem hiding this comment.
- Если у алгоритмов есть какие-то константы то стоит запаивать алгоритмы в классы и там эти параметры хранить.
- А еще глобальные константы лучше снабжать префиксом g, чтобы можно было отличать от других данных.
| namespace lp { | ||
| std::vector<std::vector<int>> MakeHelper(std::vector<std::vector<int>> &a); | ||
|
|
||
| std::vector<int> Gauss(std::vector<std::vector<int>> &a, int &cur_string_num, std::vector<std::vector<int>> &helper); |
There was a problem hiding this comment.
Мне не нравится что ты работаешь с вектором векторов явно.
Во-первых, почему не сделал отельный класс, где хранится вектор размера m * n?
Во-вторых, почему не сделал псевдоним для этого?
В-третьих, почему от int, а не от char или даже bool, если у тебя все по модулю 2?
| return helper; | ||
| } | ||
|
|
||
| std::vector<int> lp::GetContribStrings(std::vector<std::vector<int>> &a, int i) { |
There was a problem hiding this comment.
Порядок методов в cpp файле должен быть точно такой же как в h файле!
| // запустится тогда и за 5 лет Мб есть смысл сделать юзинг для инта | ||
| //Думаю гаусс одно из моих слабых мест, в том плане, что лучше переписать это на один вектор с перегрузкой оператора | ||
| //и лучше ну как-то его улучшить, хз, выглядит не идеально | ||
| std::vector<std::vector<int>> lp::MakeHelper(std::vector<std::vector<int>> &a) { |
There was a problem hiding this comment.
Не пиши lp в начале каждого метода, открой namespace в начале перед всеми методами и потом закрой его.
| break; | ||
| } | ||
| while (n1 % i == T(0) && n1 >= i) { | ||
| ans.emplace_back(i); |
There was a problem hiding this comment.
Тут нет std::move никгде в emplace_back, это нормально? Или тут T всегда маленький?
2 participants
I commited all what I had done before, except Legendre symbol (it's not working right now).
Вот это я англичанин) У меня много вопросов по коду, больше всего связано с темплейтами (я их до этого 1 раз в жизни юзал), например, когда нужно делать оберткту, а когда нет, стоит ли делать константы вида абоба = T(2), если много раз делаю T(2) или оставить это компилятору (он вроде как должен понять, что надо лишь раз вызвать конструктор класса).