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

check if config update model is same as the map surfacetype #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haider8645
Copy link
Contributor

Hello @planthaber

it was not being checked whether the map updatemodel from the 'maps/grid/MLSConfig' is the same as the SurfaceType in MLSMap.

Either we add this assert from the PR or we can remove the updateModel from the 'maps/grid/MLSConfig' because it is currently unused

Best,
Haider

@planthaber
Copy link
Member

The assert() calls are getting removed by the compiler in cmake Release builds (optoimizes), i'd rather throw an exception

@haider8645
Copy link
Contributor Author

haider8645 commented Aug 18, 2022

@planthaber added an exception. There is a slight issue with the exception thrown in the constructor and it is that the destructor is not called. If someone creates a map inside a try block and catches this exception, would it result in a memory leak? For my usecase in the MLSMapSlopeLoader this exception is not caught and the program just ends.

@chhtz
Copy link
Member

chhtz commented Dec 13, 2022

@planthaber added an exception. There is a slight issue with the exception thrown in the constructor and it is that the destructor is not called. If someone creates a map inside a try block and catches this exception, would it result in a memory leak? For my usecase in the MLSMapSlopeLoader this exception is not caught and the program just ends.

When a constructor throws, the destructor of the base class and of all member variables is still called (basically everything which is constructed before the body of the throwing constructor is executed gets destructed), so there should not be any memory leaks here.

However, I think you need to surround the log message and the throw with an if(config_.updateModel != SurfaceType) block. But if config_.updateModel is actually never used anywhere, I would also prefer to deprecate it.

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.

3 participants