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.

Sign up for GitHub

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

Jump to bottom

Add an etl::nullptr_t type to <etl/nullptr.h> #924

Merged
merged 7 commits into from
Jul 19, 2024
Merged

Add an etl::nullptr_t type to <etl/nullptr.h> #924

merged 7 commits into from
Jul 19, 2024

Conversation

tigran2008
Copy link
Contributor

@tigran2008 tigran2008 commented Jul 17, 2024 •
edited
Loading

Well, the title is self-explanatory. This is a tiny change anyway, and the code is basically what's proposed in #922.

Edit: As of this moment, I changed the pre-C++11 code from an enum into a custom class

@semanticdiff-com SemanticDiff.com
Copy link

Review changes with SemanticDiff.

@tigran2008
Copy link
Contributor Author

I'm so sorry for the mistakes, I'm a total beginner to C++ and I'm trying my best :/

@tigran2008
Copy link
Contributor Author

After I'm done, I promise I'll squash the commits 😅

@jwellbelove
Copy link
Contributor

You can run the unit tests locally if you want to check before you push, though it does take some time to do all C++11/14/17/20 variants.

@tigran2008
Copy link
Contributor Author

tigran2008 commented Jul 17, 2024 •
edited
Loading

Do you think this implementation of etl::nullptr_t is acceptable? It instantiates a _nullptr instance beforehand, and it occupies one byte of storage, unlike the previous one, which didn't occupy any (without usage). I could also do #define ETL_NULLPTR (etl::nullptr()) but I thought it wouldn't be good to instantiate a new object everywhere a nullptr is used..

Also, if you do think it's an acceptable solution, do you think I should add a (void)_nullptr; // silence unused warning? I didn't get an unused variable warning on my computer (gcc version 13.3.1 20240522 (Red Hat 13.3.1-1) (GCC) on Fedora 39) but I just noticed that it warns on https://cpp.sh

@tigran2008
etlcpp/etl issue #921 (etl::unique_ptr reset): add etl::unique_ptr(..…
af40b7f 
….)::reset(ETL_NULLPTR)

Remove default argument for the normal reset method of etl::unique_ptr (sorry, didn't notice 😬)

Silence the unused argument warning

Fix operator =(nullptr)

Replace the nullptr_t enum with a class which acts more similar to C++11 nullptr
@tigran2008
Copy link
Contributor Author

Do you think I should also delete etl::addressof(ETL_NULLPTR)?

@tigran2008
Copy link
Contributor Author

Well, I'll be waiting for a comment from you before squashing the commits

@jwellbelove
Copy link
Contributor

I'll have a look tomorrow.

jwellbelove
jwellbelove reviewed Jul 18, 2024
View reviewed changes
include/etl/memory.h Outdated Show resolved Hide resolved
@jwellbelove jwellbelove changed the base branch from master to pull-request/#924-Add-an-etl--nullptr_t-type-to-etl-nullptr.h July 19, 2024 16:20
@jwellbelove jwellbelove merged commit 22dc6b2 into ETLCPP:pull-request/#924-Add-an-etl--nullptr_t-type-to-etl-nullptr.h Jul 19, 2024
63 checks passed
jwellbelove pushed a commit that referenced this pull request Jul 25, 2024
Merge branch 'pull-request/#924-Add-an-etl--nullptr_t-type-to-etl-nul…
5885433 
…lptr.h' into development
jwellbelove pushed a commit that referenced this pull request Jul 28, 2024
@tigran2008
Add an etl::nullptr_t type to <etl/nullptr.h> (#924)
c9c0109 
* Add an etl::nullptr_t type

* etlcpp/etl issue #921 (etl::unique_ptr reset): add etl::unique_ptr(...)::reset(ETL_NULLPTR)

Remove default argument for the normal reset method of etl::unique_ptr (sorry, didn't notice 😬)

Silence the unused argument warning

Fix operator =(nullptr)

Replace the nullptr_t enum with a class which acts more similar to C++11 nullptr

* Add member pointer support and delete the addressof operator

* "Delete" etl::addressof(ETL_NULLPTR)

* Ensure compatibility with C++98

* ACTUALLY ensure compatibility with C++98

I'm stupid :/

* Correct definition according to cppreference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwellbelove jwellbelove jwellbelove left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tigran2008 @jwellbelove