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

Question about get_ref() #128

Closed
dariomt opened this issue Oct 2, 2015 · 11 comments
Closed

Question about get_ref() #128

dariomt opened this issue Oct 2, 2015 · 11 comments

Comments

@dariomt
Copy link
Contributor

dariomt commented Oct 2, 2015

I see there were some discussions about adding get_ref(), but I can't see it in the code.
What happened to get_ref()?

I believe that having get_ref() is convenient and safe. It could merely call get_ptr() and throw if it returns nullptr. It makes it easier to traverse a json object when I'm pretty sure about the types it holds. Otherwise I have to constantly check against nullptr myself.

I'd like the following code to work seamlessly:

void func(const std::string&);
json value = "foobar";
func( value.get_ref<const std::string>() );

Currently I'd have to write the following:

void func(const std::string&);
json value = "foobar";
auto p = value.get_ptr<const std::string*>();
if (!p) throw some_exception;
func( *p );

Would you accept a PR for this functionality?

@nlohmann
Copy link
Owner

nlohmann commented Oct 6, 2015

After a getter for a value and a pointer I did not follow to implement get_ref. But I would be glad to have a PR for that.

@dariomt
Copy link
Contributor Author

dariomt commented Oct 7, 2015

I'll try to provide a PR including tests and docs.

One question: what would be the preferred form?
get_ref<const std::string>()
or
get_ref<const std::string &>()

To me get_ref is enough to convey that we get a reference, the & in the template parameter is redundant (I feel the same way about the * required by get_ptr by the way).

@dariomt
Copy link
Contributor Author

dariomt commented Oct 16, 2015

I have this ready in my branch get_ref.
But I have created that branch off my master where I have all my other PRs already merged.
Do you know how I can produce a PR with only the actual changes done in my branch?

@nlohmann
Copy link
Owner

nlohmann commented Dec 7, 2015

Hi @dariomt, sorry for not answering earlier.

As for your first question: I would prefer get_ref<const std::string &> - it makes the getting of a reference explicit.

For your second question, I unfortunately have no answer.

@nlohmann
Copy link
Owner

Any news here?

@dariomt
Copy link
Contributor Author

dariomt commented Dec 16, 2015

My changes already implemented get_ref<const std::string &>, so that's good.

However, I'm still trying to get the branch in proper shape to make the PR easy for you, git newbie here :)

@nlohmann
Copy link
Owner

Ok - looking forward to it!

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Dec 28, 2015
@nlohmann
Copy link
Owner

@dariomt - any news on this?

@dariomt
Copy link
Contributor Author

dariomt commented Jan 18, 2016

Sorry, I haven't had time to look into this.
I should have time this week.

@dariomt
Copy link
Contributor Author

dariomt commented Jan 18, 2016

PR ready!
I think I've merged everything correctly.

@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Jan 20, 2016
@nlohmann nlohmann added this to the Release 1.0.1 milestone Jan 20, 2016
@nlohmann
Copy link
Owner

Thanks @dariomt for the good job!

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

No branches or pull requests

2 participants