pteros icon indicating copy to clipboard operation
pteros copied to clipboard

Quick review

Open guffy1234 opened this issue 4 months ago • 2 comments

Hi

Just result of pretty short review:

  1. строки не надо инитить "". это ненужные выделения памяти. лучше пусть парсер понимает эмпти стринг. так же как и парсеру вроде не надо делать постоянно ресет в конструкторах - он же и так новый, не? в методах встречается посторная инициализвация полей возвращаемого нового селекшена хотя он только после конструктора
  2. зачем индексы и номера фрейма проверять на отрицательность если можно просто юзать беззнаковый size_t. вообще там местами вольный обмен между инт и сайз_т шо как бы не альо.
  3. во многих операциях с временными векторами не юзается мув семантика. зачем временный вектор копировать в _индекс если его можно мувнуть?
  4. селект(стринг) принимает строку не по ссылке. но это мелочи. в процессе экспанда макросов регеспы парсятся снова и снова. почему не сделать статик массив готовых распасенных? они ж потом константно юзаются
  5. в каком месте парсера модифицируется стартинг сабсет, шо его так уж надо передавать некостантным? тоже самое - зачем парсеру селекшена неконстантны систем?
  6. вообще много вольного обращения с конст кастом. делаем внутри неконстантный поинтер и потом в него постоянно кастим из конста. как по мне лучше сделать константный поинтер и кастить только там где это действительно нужно. заодно будет понятно где именно нужна неконстантность.
  7. возможно я бы так не мучал атом_хендлер, а сделал пару атом_хендлер и конст_атом_хендлер. и возвращал тот что нужно в зависимости от того константный индексатор или нет
  8. поскольку вектор индексов сортированный - иногда можно пользоваться вставкой через лоу_баунд чем пихать в конец и по большому кругу
  9. Selection Selection::select(int ind1, int ind2) const дублирует код - уже есть такой конструктор
  10. лютый копипаст кода проверок хотя можно просто сделать впомогательные верифи (инлайн?) методы. в теории, кстати, вместо налл поинтера на систем для пустого селекшена можно юзать статик экземпляр системы который будет считатся за "пустой"
  11. какой смысл делать Affine3f fit_transform(const Selection& sel1, const Selection& sel2, bool translate_to_zero) и внутри конст каст, если походу все равно никогда он не вызывается для константных селекшенов?

guffy1234 avatar Feb 12 '24 17:02 guffy1234