Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quick review #28

Open
guffy1234 opened this issue Feb 12, 2024 · 2 comments
Open

Quick review #28

guffy1234 opened this issue Feb 12, 2024 · 2 comments

Comments

@guffy1234
Copy link

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
Copy link
Author

guffy1234 commented Feb 12, 2024

  1. Selection::invert() проще писать в новый пустой вектор а потом его свапнуть с _индекс. тоже самое System::filter_atoms()

@guffy1234
Copy link
Author

  1. вообще если пристально пройти по коду и применить мув семантику то не было бы таких кусков. когда мы деаем фрейм, потом его копию пушим в вектов, а потом парсим в последний элемент.
    Frame fr;
    frame_append(fr);
    f->read(this, &frame(num_frames()-1), c);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant