[trivial] Add end of namespace comments. Improve consistency. #9544

practicalswift · 2017-01-13T19:23:47Z

No description provided.

practicalswift · 2017-01-31T20:02:57Z

Merge conflict resolved!

jtimon · 2017-02-01T00:19:50Z

src/base58.cpp

@@ -225,7 +225,7 @@ class CBitcoinAddressVisitor : public boost::static_visitor<bool>
    bool operator()(const CNoDestination& no) const { return false; }
 };

-} // anon namespace
+} // namespace


why remove anon ?

@jtimon My reasoning behind that was to achieve consistency how anonymous namespaces are indicated. Consistency in order to improve readability and allow for automatic identification of missing/incorrect end of namespace comments.

// namespace is the form preferred by the clang-tooling (clang-tidy, etc). // anonymous namespace is also quite common. I haven't seen // anon namespace before, but let me know if the choice of // anon namespace is intentional and I'll adjust this PR :-)

Well, it just seems to me that the same consistency can be achieved leaving "anon namespace" for anonymous namespaces with less disruption. But if clang prefers // namespace or // anonymous namespace I don't have a strong opinion.

practicalswift · 2017-02-27T22:19:43Z

Any changes needed before merge? :-)

paveljanik · 2017-02-28T12:44:01Z

The question should read: How can I attract you to the review? My answer is: sorry, I do not know.

I'm pro // anonymous namespace and having the end of namespace marks. But we do not have a recommendation for this in the developer notes, there is only "Block style example:" section there. With no end of namespace comment...

practicalswift · 2017-02-28T13:04:00Z

@paveljanik I see! If the consensus opinion is that end of namespace marks are optional then this PR is redundant and I'll close it :-)

laanwj · 2017-02-28T13:32:59Z

I think the point is that if end-of-namespace comments are to be required, they should be mentioned in doc/developer-notes.md along with rationale. It should be clear to contributors what the "rules" are. If not, changes like this come out of the blue.

paveljanik · 2017-02-28T14:34:15Z

@practicalswift I do now know what the consensus is, but my opinion is the same as @laanwj 's.

practicalswift · 2017-02-28T15:29:48Z

@laanwj Good point! I've added a note to doc/developer-notes.md in this PR.

paveljanik · 2017-02-28T17:24:15Z

I'd slightly prefer // anonymous namespace over // namespace at the end of anonymous namespace, but this is minor.

practicalswift · 2017-03-01T08:22:43Z

@paveljanik It seems like // namespace is the dominant form currently used in the repo:

$ git checkout master
$ git grep -E '} *// namespace$' | wc -l
21
$ git grep -E '} *// anonymous namespace$' | wc -l
3

// namespace is also the form preferred by the clang-tooling (clang-tidy, etc).

paveljanik · 2017-03-01T08:25:14Z

@practicalswift yes. But it seems like the author forget to add the real name of the namespace after it ;-)

paveljanik · 2017-03-01T08:27:30Z

src/univalue/lib/univalue.cpp

@@ -71,7 +71,7 @@ bool ParseDouble(const std::string& str, double *out)
    if(out) *out = result;
    return text.eof() && !text.fail();
 }
-}
+} // namespace


This is univalue -> please report upstream.

paveljanik · 2017-03-01T08:28:35Z

Concept ACK.

Please remove univalue change. Then utACK

practicalswift · 2017-03-01T08:44:20Z

@paveljanik Removed univalue change as requested :-)

paveljanik · 2017-03-01T17:05:16Z

ACK 8530606

Thanks!

practicalswift · 2017-03-28T16:52:51Z

@paveljanik Thanks for reviewing! Let me know if any further tweaks are needed :-)

practicalswift · 2017-04-27T22:52:12Z

Conflicts resolved!

jtimon · 2017-04-27T23:53:36Z

Fast re-review checked that still doesn't need testing ACK 486a5d7

jtimon · 2017-05-31T16:55:52Z

doc/developer-notes.md

+- Terminate namespaces with a comment (`// namespace mynamespace`). The comment
+  should be placed on the same line as the brace closing the namespace, e.g.
+
+  ```c++


nit: extra spaces?

@jtimon Sorry I'm not sure I follow - where should the extra spaces be placed? :-)

s/ ```/```/ ?

EDIT: not important

practicalswift · 2017-05-31T20:22:02Z

@jtimon Fixed! :-)

laanwj · 2017-06-26T11:40:23Z

utACK 5a9b508

…ency. 5a9b508 [trivial] Add end of namespace comments (practicalswift) Tree-SHA512: 92b0fcae4d1d3f4da9e97569ae84ef2d6e09625a5815cd0e5f0eb6dd2ecba9852fa85c184c5ae9de5117050330ce995e9867b451fa8cd5512169025990541a2b

…consistency. 5a9b508 [trivial] Add end of namespace comments (practicalswift) Tree-SHA512: 92b0fcae4d1d3f4da9e97569ae84ef2d6e09625a5815cd0e5f0eb6dd2ecba9852fa85c184c5ae9de5117050330ce995e9867b451fa8cd5512169025990541a2b

practicalswift changed the title ~~Add end of namespace comments. Improve consistency for anonymous namespaces.~~ [trivial] Add end of namespace comments. Improve consistency. Jan 13, 2017

fanquake added the Docs label Jan 13, 2017

practicalswift mentioned this pull request Jan 28, 2017

[refactor] Remove using namespace <xxx> from src/ #9644

Merged

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from 2553d2b to 7140b87 Compare January 31, 2017 20:02

jtimon reviewed Feb 1, 2017

View reviewed changes

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch 4 times, most recently from 15bd40f to 602d1aa Compare February 28, 2017 15:27

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from 602d1aa to 11d4870 Compare February 28, 2017 15:32

paveljanik reviewed Mar 1, 2017

View reviewed changes

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from 11d4870 to 8530606 Compare March 1, 2017 08:41

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from 470a0ee to c629d4f Compare April 27, 2017 22:44

practicalswift closed this Apr 27, 2017

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from c629d4f to 4c92401 Compare April 27, 2017 22:48

practicalswift reopened this Apr 27, 2017

jtimon reviewed May 31, 2017

View reviewed changes

[trivial] Add end of namespace comments

5a9b508

practicalswift force-pushed the consistent-use-of-end-of-namespace-comments branch from 486a5d7 to 5a9b508 Compare May 31, 2017 20:21

laanwj merged commit 5a9b508 into bitcoin:master Jun 26, 2017

meshcollider mentioned this pull request Jun 29, 2017

[Trivial] Improve end-of-namespace comment consistency #10702

Closed

practicalswift deleted the consistent-use-of-end-of-namespace-comments branch April 10, 2021 19:31

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[trivial] Add end of namespace comments. Improve consistency. #9544

[trivial] Add end of namespace comments. Improve consistency. #9544

practicalswift commented Jan 13, 2017

practicalswift commented Jan 31, 2017

jtimon Feb 1, 2017

practicalswift Feb 1, 2017 •

edited

jtimon Feb 2, 2017

practicalswift commented Feb 27, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Feb 28, 2017

laanwj commented Feb 28, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Feb 28, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Mar 1, 2017

paveljanik commented Mar 1, 2017

paveljanik Mar 1, 2017

paveljanik commented Mar 1, 2017

practicalswift commented Mar 1, 2017

paveljanik commented Mar 1, 2017

practicalswift commented Mar 28, 2017

practicalswift commented Apr 27, 2017

jtimon commented Apr 27, 2017 •

edited

jtimon May 31, 2017

practicalswift May 31, 2017

jtimon May 31, 2017 •

edited

practicalswift commented May 31, 2017

laanwj commented Jun 26, 2017

[trivial] Add end of namespace comments. Improve consistency. #9544

[trivial] Add end of namespace comments. Improve consistency. #9544

Conversation

practicalswift commented Jan 13, 2017

practicalswift commented Jan 31, 2017

jtimon Feb 1, 2017

Choose a reason for hiding this comment

practicalswift Feb 1, 2017 • edited

Choose a reason for hiding this comment

jtimon Feb 2, 2017

Choose a reason for hiding this comment

practicalswift commented Feb 27, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Feb 28, 2017

laanwj commented Feb 28, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Feb 28, 2017

paveljanik commented Feb 28, 2017

practicalswift commented Mar 1, 2017

paveljanik commented Mar 1, 2017

paveljanik Mar 1, 2017

Choose a reason for hiding this comment

paveljanik commented Mar 1, 2017

practicalswift commented Mar 1, 2017

paveljanik commented Mar 1, 2017

practicalswift commented Mar 28, 2017

practicalswift commented Apr 27, 2017

jtimon commented Apr 27, 2017 • edited

jtimon May 31, 2017

Choose a reason for hiding this comment

practicalswift May 31, 2017

Choose a reason for hiding this comment

jtimon May 31, 2017 • edited

Choose a reason for hiding this comment

practicalswift commented May 31, 2017

laanwj commented Jun 26, 2017

practicalswift Feb 1, 2017 •

edited

jtimon commented Apr 27, 2017 •

edited

jtimon May 31, 2017 •

edited