Skip to content

Conversation

@supraja777
Copy link

Worked on memory leak issue - 75

@vil02
Copy link
Contributor

vil02 commented Oct 5, 2024

The address sanitizer still reports several memory leaks. You can see it on your own by adding -fsanitize=address to:

CFLAGS := -std=c++17 -MMD -MP

Remember to run make clean.

From what I see, all of these problems can be resolved using std::unique_ptr.


DreeNode(std::string &str);

~DreeNode() {
Copy link
Owner

@ujjwall-R ujjwall-R Oct 6, 2024

Choose a reason for hiding this comment

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

wont it increase the latency? Latency is already very high due to iterations. Lets not do this part. We will have to find some other way. Maybe concurrently. There is a separate issue for that.

Can we have some functionality like finally block in cpp (like what we have in python)? Lets figure out some doing it.

Copy link
Author

Choose a reason for hiding this comment

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

Sure will look for other possibilities and discuss

#include <iostream>
vector<pair<int, DreeNode *>> SearchDirectory::search(DreeNode *root, string &query, DreeHelpersI *dreeHelpers) {
vector<pair<int, DreeNode *>> searchResult;
vector<pair<int, pair<string, string>>> SearchDirectory::search(DreeNode *root, string &query, DreeHelpersI *dreeHelpers) {
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't it decreasing the readability of the function?

Copy link
Author

Choose a reason for hiding this comment

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

Do you want the function signature to remain as - vector<pair<int, DreeNode *>> @ujjwall-R

Copy link
Owner

Choose a reason for hiding this comment

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

yes, it make sure the function is readable.

@ujjwall-R
Copy link
Owner

Also, change the PR merge to dev branch. We will merge in main once we move for release after thorough testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants