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

Remove unneccessary NULL check in srs_freep. v5.0.150, v6.0.38 #3477

Merged
merged 4 commits into from
Mar 25, 2023
Merged

Remove unneccessary NULL check in srs_freep. v5.0.150, v6.0.38 #3477

merged 4 commits into from
Mar 25, 2023

Conversation

yashwardhan-jyani
Copy link
Contributor

Removed the unnecessary NULL pointer checks in trunk/src/core/srs_core.hpp

https://isocpp.org/wiki/faq/freestore-mgmt#delete-handles-null

This is my first Pull Request, So if anything's wrong please tell me

Copy link

@elfring elfring left a comment

Choose a reason for hiding this comment

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

The commit message can be improved.

@winlinvip winlinvip force-pushed the develop branch 3 times, most recently from c056094 to 2ac9eb8 Compare March 23, 2023 06:02
@winlinvip winlinvip changed the title remove null-pointer-check Remove unneccessary NULL check in srs_freep. Mar 23, 2023
@winlinvip
Copy link
Member

Seems make sense to me, however I wonder if all compiler check the null pointer?

@elfring
Copy link

elfring commented Mar 23, 2023

..., however I wonder if all compilers check the null pointer?

💭 Would you like to support any compiler versions which would be standard-incompliant in this technical detail?

TRANS_BY_GPT3

@xiaozhihong
Copy link
Collaborator

@yashwardhan-jyani

Thanks for your PR.

Follow the reference, delete nullptr is safe.
https://cplusplus.com/reference/new/operator%20delete/
image

https://en.cppreference.com/w/cpp/memory/new/operator_delete
image

And I test it with simple code.

#include <iostream>

int main()
{
    char* p = NULL;
    delete p;

    return 0;
}

It works well.

Then I gdb the program and find the implement of delete in gcc.
image

image

Obvious, operator delete call std::free, std::free call libc free directly, and man 3 free tell us
image

But I still have a question, in other platforms which not use gcc, for example, MSVC of Windows, cause the same result? Can you test it? Thanks.

@elfring
Copy link

elfring commented Mar 24, 2023

But I still have a question, ..., cause the same result?

How much do you care for standard compliance of applied compiler variants? 🤔

TRANS_BY_GPT3

@yashwardhan-jyani
Copy link
Contributor Author

@xiaozhihong

I tried this on MSVC compiler and it's working fine.

Screenshot 2023-03-24 145526

Ignore the errors, it's the intelliSense, i tried to get rid of them but can't seem to do so.
cx

@winlinvip winlinvip requested a review from xiaozhihong March 25, 2023 01:38
@winlinvip winlinvip changed the title Remove unneccessary NULL check in srs_freep. Remove unneccessary NULL check in srs_freep. v5.0.149, v6.0.37 Mar 25, 2023
@winlinvip winlinvip changed the title Remove unneccessary NULL check in srs_freep. v5.0.149, v6.0.37 Remove unneccessary NULL check in srs_freep. Mar 25, 2023
@winlinvip winlinvip changed the title Remove unneccessary NULL check in srs_freep. Remove unneccessary NULL check in srs_freep. v5.0.150, v6.0.38 Mar 25, 2023
@winlinvip winlinvip merged commit b574ad1 into ossrs:develop Mar 25, 2023
winlinvip added a commit that referenced this pull request Mar 25, 2023
PICK b574ad1

Co-authored-by: winlin <winlin@vip.126.com>
Co-authored-by: john <hondaxiao@tencent.com>
duiniuluantanqin pushed a commit to duiniuluantanqin/srs that referenced this pull request Mar 31, 2023
…#3477)

Co-authored-by: winlin <winlin@vip.126.com>
Co-authored-by: john <hondaxiao@tencent.com>
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants