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

Possible clarification and reimplementation of Ref<T> classes #719

Merged
merged 2 commits into from
Aug 12, 2023

Conversation

mr-mocap
Copy link
Contributor

@mr-mocap mr-mocap commented Aug 2, 2023

I was a bit confused by the current implementation of the ConstRef and Ref classes, so I decided to see if I could clarify your original intent.

I hated to "mess up" the simplicity of the original classes, but I think I've codified what you were trying to do.

I'm hoping you will find this useful enough to merge into your repo.

@ArthurSonzogni
Copy link
Owner

Thanks! I like it!
I would like to make some small tweaks, but other it looks good to me.
I am quite busy at the moment, so please expect some delays.

@mr-mocap
Copy link
Contributor Author

mr-mocap commented Aug 4, 2023

Excellent!

I was going to make some more changes to the StringRef and ConstStringRef classes, mainly seeing if I could make it a variant of a std::string and a std::string_view, as this also seems to be in the spirit of either wanting to initialize with a std::string or a hard-coded const char * string ("like this"). In this way, only an immutable string_view would mostly be passed around, which is really lightweight.

Don't worry about immediate responses, as I prefer quality over most other things. This may take some back and forth to really get it polished.

@ArthurSonzogni
Copy link
Owner

  • I removed the c++20 check in example, because we are already using it.
  • I added operator= for Ref.
  • I removed some std::move when clang-tidy suggested it was not useful.
  • I simplified some things.

Does it looks good to you?

@mr-mocap
Copy link
Contributor Author

mr-mocap commented Aug 9, 2023

It looks good so far.
I have changed the implementation of ConstStringRef on my side, but I cannot figure out how to get that change into this pull request.
Perhaps I should just make another pull request after this closes.

@mr-mocap
Copy link
Contributor Author

mr-mocap commented Aug 9, 2023

Found out how!
Try this out...

@ArthurSonzogni
Copy link
Owner

Thanks!
I think you overrode my last change. I will push again what I did, and if you think there are something to tweak, I will let you publish another PR.

@mr-mocap mr-mocap closed this Aug 12, 2023
@ArthurSonzogni ArthurSonzogni merged commit 06ba1c1 into ArthurSonzogni:main Aug 12, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants