Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

new w/ OOM now aborts by defaults, or throw an exception#7536

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
devyte merged 11 commits intoesp8266:masterfromd-a-v:stdnothrow
Aug 31, 2020

Conversation

@d-a-v
Copy link
Collaborator

@d-a-vd-a-v commentedAug 17, 2020
edited
Loading

Changes:

  • remove legacynew behavior, c++operator new is supposed tonever returnnullptr
    • when exceptions are enabled: a catchable exception is raised
    • when exceptions are disabled (defaults): abort is called: caller address is showed for debuggers
  • unusedarduino_new has exactly the same behavior asnew (std::nothrow), removing the first.
  • all source code withnew which are checking the returned value are now callingnew (std::nothrow) instead
    (the others will abort on oom, then reboot)
  • malloc is untouched: it returnsnullptr on oom likenew (std::nothrow)

@d-a-vd-a-v changed the title(WIP) replacenew bynew (std::nothrow), removearduino_newreplacenew bynew (std::nothrow), removearduino_newAug 23, 2020
@d-a-vd-a-v changed the titlereplacenew bynew (std::nothrow), removearduino_newnew w/ OOM now aborts by defaults, or throw an exceptionAug 24, 2020
@TD-er
Copy link
Contributor

Can we also have a build option to deliberately keep the old situation?
Or else my code will cause crashes where it is checking the returned pointer as soon as this gets merged.
And right now I cannot change the code to callnew(std::nothrow) as it is not yet merged.
I may need to make a macro for it or else my code will become quite messy while I still want to support old and new core libs.

@d-a-v
Copy link
CollaboratorAuthor

new (std::nothrow) is currently supposed to work. Did you try it ?

@devyte
Copy link
Collaborator

@TD-er the whole point of this PR is to clean up the current situation and better conform to the C++ language standard. That means removing the legacy behavior.
Although this is expected to be merged soon, you have ample time to migrate, assuming you really need it.
@d-a-v 's question is valid: new(std::nothrow) is supposed to work now. If it doesn't, We Need To Know.

@TD-er
Copy link
Contributor

I have not tried it yet.
I just assumed it wouldn't because this PR implemented it...

I will try to see if it compiles using the current core code and will start making a macro for it.

@TD-er
Copy link
Contributor

OK, next time I should try it first before panic.
It does compile just fine, and the changes do appear quite easy to do in my code (not that manynew calls)

Copy link
Collaborator

@earlephilhowerearlephilhower left a comment

Choose a reason for hiding this comment

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

Looks very nice! Smart move to overload the new(std::nothrow&) operator, I was wondering how you'd fix things.

@devyte
Copy link
Collaborator

Thanks for this, finally this is cleaned up!

@devytedevyte merged commit5b767a3 intoesp8266:masterAug 31, 2020
@devytedevyte added this to the3.0.0 milestoneAug 31, 2020
if (ctx)
delete ctx;
if (sha256)
delete sha256;
Copy link
Collaborator

Choose a reason for hiding this comment

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

isif (...) redundant here? afaik, delete will be no-op with nullptr

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

We don't know which one is nullptr

Copy link
Collaborator

Choose a reason for hiding this comment

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

meaning, we can delete both. one nullptr, another created (or both nullptr)
i.e.https://godbolt.org/z/63PKa3

d-a-v reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@mcsprmcsprmcspr left review comments

@earlephilhowerearlephilhowerearlephilhower approved these changes

@devytedevytedevyte approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@d-a-v@TD-er@devyte@earlephilhower@mcspr

[8]ページ先頭

©2009-2025 Movatter.jp