Skip to content
This repository has been archived by the owner on Feb 15, 2024. It is now read-only.

Add doxygen support, all ready just need good comments now #37

Closed
wants to merge 8 commits into from

Conversation

yinyunqiao
Copy link

No description provided.

@jgarzik
Copy link
Owner

jgarzik commented May 8, 2017

Good progress :)

lib/univalue.cpp Outdated
for (std::map<std::string,UniValue::VType>::const_iterator it = t.begin();
it != t.end(); ++it) {
size_t idx = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Comments:

  1. Please put into a separate pull request, as this is unrelated to documentation changes.

  2. Let the compiler perform the hoisting. No need to do it manually.

General rule: Limit scope of a variable, and let the compiler figure out the rest. If it becomes a performance issue notable on profiles, revisit.

lib/univalue.cpp Outdated
@@ -163,6 +163,9 @@ bool UniValue::findKey(const std::string& key, size_t& retIdx) const

bool UniValue::checkObject(const std::map<std::string,UniValue::VType>& t)
{
if (typ != VOBJ)
return false;

size_t idx = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

Agree with this change - but please do it outside the doxygen pull request. Thanks!

jgarzik added a commit that referenced this pull request Aug 3, 2017
pushKV: Behave properly in the face of duplicate keys.

checkObject: return error, if not object.

Inspired-by: Charles Yin <[email protected]> in PR #37
@jgarzik
Copy link
Owner

jgarzik commented Aug 3, 2017

Merged a modified version of your unrelated-to-doxygen changes as ceb1194

@jgarzik jgarzik mentioned this pull request Feb 3, 2019
@jgarzik jgarzik added this to the 2.0.0 milestone Feb 3, 2019
@jgarzik jgarzik closed this Feb 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants