Skip to content

Conversation

@graag
Copy link

@graag graag commented Oct 29, 2025

  • Dodałem delete tam gdzie digit był tworzony a następnie nie jest zapisywany w DigitCollection
  • Gdzie się dało zamieniłem na alokowanie zmiennych na stosie
  • Przepisałem GateVolumeID tak by używał kompozycji, oryginalnie dziedziczył po std::vector co nie jest dobrą praktyką skoro std::vector nie ma wirtualnego destruktora. Ponieważ był memory leak miałem podejrzenie że może gdzieś ktoś trzyma wskaźnik jako typ *std::vector i to generuje problemy. W efekcie nic się nie zmieniło. Myślę że można założyć że nie jest tu używany polimorfizm, więc się nie upieram żeby zostawić tą zmianę.

@graag graag requested a review from wkrzemien October 29, 2025 08:53
@graag
Copy link
Author

graag commented Oct 29, 2025

@wkrzemien Prośba o review :)

Copy link

@wkrzemien wkrzemien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ogołnie nie mam większych uwag. Jeżeli chcesz zostawić interfejs vector, to dodałbym przynajmniej implementacje end().
tam gdzie używasz else, to dodaj{} będzie bardziej konsystentnie z istniejącym kodem a mnie Pythonowo. Dałbym też po delete ustawianie nullptr.


virtual inline ~GateVolumeID() {}

public: // vector interface

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To mi się tak średnio podoba. Generalnie pomysł fajny i to co dodałeś też jest ok, tylko że użytkownik mógłby oczekiwać bardziej kompletnego interfejsu, żeby to się zachowywało jako vector. Na pewno brakuje implementacji end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Wydzielę to jako oddzielny PR i dodam ten end.

- Add delete statements for digits that are not stored in geant gigit
  storage
- Convert GateVolumeID from std::vector inheritance to composition
@graag graag force-pushed the digits-memleak-fixes branch from a72f82e to 92c23d6 Compare January 21, 2026 08:31
@graag
Copy link
Author

graag commented Jan 22, 2026

@graag graag closed this Jan 22, 2026
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

Successfully merging this pull request may close these issues.

4 participants