[trivial] Add end of namespace comments. Improve consistency. by practicalswift · Pull Request #9544 · bitcoin/bitcoin

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

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

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

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

src/univalue/lib/univalue.cpp Outdated

Copy link

Contributor

paveljanik Mar 1, 2017

Choose a reason for hiding this comment

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

This is univalue -> please report upstream.

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

Conversation

practicalswift commented Jan 13, 2017

Uh oh!

practicalswift commented Jan 31, 2017

Uh oh!

jtimon Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 27, 2017

Uh oh!

paveljanik commented Feb 28, 2017

Uh oh!

practicalswift commented Feb 28, 2017

Uh oh!

laanwj commented Feb 28, 2017

Uh oh!

paveljanik commented Feb 28, 2017

Uh oh!

practicalswift commented Feb 28, 2017

Uh oh!

paveljanik commented Feb 28, 2017

Uh oh!

practicalswift commented Mar 1, 2017

Uh oh!

paveljanik commented Mar 1, 2017

Uh oh!

paveljanik Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Mar 1, 2017

Uh oh!

practicalswift commented Mar 1, 2017

Uh oh!

paveljanik commented Mar 1, 2017

Uh oh!

practicalswift commented Mar 28, 2017

Uh oh!

practicalswift commented Apr 27, 2017

Uh oh!

jtimon commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon May 31, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift May 31, 2017

Choose a reason for hiding this comment

Uh oh!

jtimon May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 31, 2017

Uh oh!

laanwj commented Jun 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

practicalswift Feb 1, 2017 •

edited

Loading

jtimon commented Apr 27, 2017 •

edited

Loading

jtimon May 31, 2017 •

edited

Loading