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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:consistent-use-of-end-of-namespace-comments changing 20 files +37 −22
  1. practicalswift commented at 7:23 PM on January 13, 2017: contributor

    No description provided.

  2. practicalswift renamed this:
    Add end of namespace comments. Improve consistency for anonymous namespaces.
    [trivial] Add end of namespace comments. Improve consistency.
    on Jan 13, 2017
  3. fanquake added the label Docs and Output on Jan 13, 2017
  4. practicalswift force-pushed on Jan 31, 2017
  5. practicalswift commented at 8:02 PM on January 31, 2017: contributor

    Merge conflict resolved!

  6. in src/base58.cpp:None in 7140b876d5 outdated 
     224 | @@ -225,7 +225,7 @@ class CBitcoinAddressVisitor : public boost::static_visitor<bool>
 225 |      bool operator()(const CNoDestination& no) const { return false; }
 226 |  };
 227 |  
 228 | -} // anon namespace
 229 | +} // namespace
    jtimon commented at 12:19 AM on February 1, 2017:

    why remove anon ?

    practicalswift commented at 6:53 AM on February 1, 2017:

    @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 :-)

    jtimon commented at 8:04 PM on February 2, 2017:

    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.

  7. practicalswift commented at 10:19 PM on February 27, 2017: contributor

    Any changes needed before merge? :-)

  8. paveljanik commented at 12:44 PM on February 28, 2017: contributor

    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...

  9. practicalswift commented at 1:04 PM on February 28, 2017: contributor

    @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 :-)

  10. laanwj commented at 1:32 PM on February 28, 2017: member

    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.

  11. paveljanik commented at 2:34 PM on February 28, 2017: contributor

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

  12. practicalswift force-pushed on Feb 28, 2017
  13. practicalswift force-pushed on Feb 28, 2017
  14. practicalswift force-pushed on Feb 28, 2017
  15. practicalswift force-pushed on Feb 28, 2017
  16. practicalswift commented at 3:29 PM on February 28, 2017: contributor

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

  17. practicalswift force-pushed on Feb 28, 2017
  18. paveljanik commented at 5:24 PM on February 28, 2017: contributor

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

  19. practicalswift commented at 8:22 AM on March 1, 2017: contributor

    @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).

  20. paveljanik commented at 8:25 AM on March 1, 2017: contributor

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

  21. in src/univalue/lib/univalue.cpp:None in 11d487025f outdated 
      70 | @@ -71,7 +71,7 @@ bool ParseDouble(const std::string& str, double *out)
  71 |      if(out) *out = result;
  72 |      return text.eof() && !text.fail();
  73 |  }
  74 | -}
  75 | +} // namespace
    paveljanik commented at 8:27 AM on March 1, 2017:

    This is univalue -> please report upstream.

  22. paveljanik commented at 8:28 AM on March 1, 2017: contributor

    Concept ACK.

    Please remove univalue change. Then utACK

  23. practicalswift force-pushed on Mar 1, 2017
  24. practicalswift commented at 8:44 AM on March 1, 2017: contributor

    @paveljanik Removed univalue change as requested :-)

  26. practicalswift commented at 4:52 PM on March 28, 2017: contributor

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

  27. practicalswift force-pushed on Apr 27, 2017
  28. practicalswift closed this on Apr 27, 2017
  29. practicalswift force-pushed on Apr 27, 2017
  30. practicalswift reopened this on Apr 27, 2017
  31. practicalswift commented at 10:52 PM on April 27, 2017: contributor

    Conflicts resolved!

  32. jtimon commented at 11:53 PM on April 27, 2017: contributor

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

  33. in doc/developer-notes.md:414 in 486a5d731b outdated 
     407 | @@ -408,6 +408,21 @@ Source code organization
 408 |  
 409 |    - *Rationale*: Avoids symbol conflicts
 410 |  
 411 | +- Terminate namespaces with a comment (`// namespace mynamespace`). The comment
 412 | +  should be placed on the same line as the brace closing the namespace, e.g.
 413 | +
 414 | +  ```c++
    jtimon commented at 4:55 PM on May 31, 2017:

    nit: extra spaces?

    practicalswift commented at 6:56 PM on May 31, 2017:

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

    jtimon commented at 8:10 PM on May 31, 2017:

    s/ ```/```/ ?

    EDIT: not important

  34. [trivial] Add end of namespace comments 5a9b508279
  35. practicalswift force-pushed on May 31, 2017
  36. practicalswift commented at 8:22 PM on May 31, 2017: contributor

    @jtimon Fixed! :-)

  37. laanwj commented at 11:40 AM on June 26, 2017: member

    utACK 5a9b508

  38. laanwj merged this on Jun 26, 2017
  39. laanwj closed this on Jun 26, 2017
  40. laanwj referenced this in commit f3f1e2e7d3 on Jun 26, 2017
  41. PastaPastaPasta referenced this in commit 2ce5560815 on Jul 6, 2019
  42. PastaPastaPasta referenced this in commit 7f7733315f on Jul 6, 2019
  43. PastaPastaPasta referenced this in commit 3caaf832dd on Jul 8, 2019
  44. PastaPastaPasta referenced this in commit 28c7e5d515 on Jul 9, 2019
  45. PastaPastaPasta referenced this in commit f2a4776464 on Jul 11, 2019
  46. barrystyle referenced this in commit 573256d719 on Jan 22, 2020
  47. practicalswift deleted the branch on Apr 10, 2021
  48. DrahtBot locked this on Aug 18, 2022
Contributors
practicalswiftjtimonpaveljaniklaanwj
Labels
Docs
Linked (view graph)
#10702 [Trivial] Improve end-of-namespace comment consistency
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me