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

refactor: Remove 9 year old comment in memdebug.c #5973

Closed

Conversation

@emilengler
Copy link
Contributor

@emilengler emilengler commented Sep 17, 2020

The comment described a way how an ancient libcurl version handled
memory. 9 years later this has no value to be in there. If people need
to know about it they can have a look at VCS logs.

emilengler added a commit to emilengler/curl that referenced this pull request Sep 17, 2020
The comment described a way how an ancient libcurl version handled
memory. 9 years later this has no value to be in there. If people need
to know about it they can have a look at VCS logs.

Closes curl#5973
@emilengler emilengler force-pushed the emilengler:2020-09-refactor-memdebug-comment branch from 16693c6 to 93bd41c Sep 17, 2020
@bagder
Copy link
Member

@bagder bagder commented Sep 17, 2020

I think you should consider removing the entire memfill functionality if you remove the backstory/docs for it.

That custom memory fill thing has lost its purpose. valgrind and ASAN are superior tools to catch memory issues.

@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Sep 22, 2020

I will try to work on this until Saturday. I am currently a bit busy

emilengler added a commit to emilengler/curl that referenced this pull request Sep 22, 2020
The used to be away how an ancient libcurl version handled
memory. 9 years later this has no value to be in there. If people need
to know about it they can have a look at VCS logs.

Closes curl#5973
@emilengler emilengler force-pushed the emilengler:2020-09-refactor-memdebug-comment branch from 93bd41c to bcc3fe9 Sep 22, 2020
@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Sep 22, 2020

Found some time actually! This is ready for merge I think

@bagder
Copy link
Member

@bagder bagder commented Sep 22, 2020

You need to remove the use of those macros too:

memdebug.c:131:5: error: implicit declaration of function 'mt_malloc_fill' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    mt_malloc_fill(mem->mem, wantedsize);
    ^
memdebug.c:279:5: error: implicit declaration of function 'mt_free_fill' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    mt_free_fill(mem->mem, mem->size);
    ^
2 errors generated.
@emilengler
Copy link
Contributor Author

@emilengler emilengler commented Sep 23, 2020

You need to remove the use of those macros too:


memdebug.c:131:5: error: implicit declaration of function 'mt_malloc_fill' is invalid in C99 [-Werror,-Wimplicit-function-declaration]

    mt_malloc_fill(mem->mem, wantedsize);

    ^

memdebug.c:279:5: error: implicit declaration of function 'mt_free_fill' is invalid in C99 [-Werror,-Wimplicit-function-declaration]

    mt_free_fill(mem->mem, mem->size);

    ^

2 errors generated.

Sure... but my cc haven't reported a single error on that patch 😟

@bagder
Copy link
Member

@bagder bagder commented Sep 23, 2020

Then you must've forgot to build it debug-enabled - as you can see in the CI...

The used to be away how an ancient libcurl version handled
memory. 9 years later this has no value to be in there. If people need
to know about it they can have a look at VCS logs.

Closes #5973
@emilengler emilengler force-pushed the emilengler:2020-09-refactor-memdebug-comment branch from bcc3fe9 to 97f3657 Sep 28, 2020
@bagder bagder closed this in 82d66f1 Sep 28, 2020
@bagder
Copy link
Member

@bagder bagder commented Sep 28, 2020

Thanks! (I edited the commit message somewhat)

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.