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.

Sign up for GitHub

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

Jump to bottom

[chrono] Use nested namespace definitions in the library where applicable #4256

Closed
zygoloid opened this issue Sep 30, 2020 · 3 comments · Fixed by #5114
Closed

[chrono] Use nested namespace definitions in the library where applicable #4256

zygoloid opened this issue Sep 30, 2020 · 3 comments · Fixed by #5114
Assignees
@jensmaurer

Comments

@zygoloid
Copy link
Member

For example, in [chrono.syn] we have:

inline namespace literals {
inline namespace chrono_literals {
  ...
}
}

(Note that namespace inline literals::inline chrono_literals is ill-formed: there's no way to collapse two levels of namespace where the outer level is inline.)

I wonder if it would be better to express [chrono.syn] as:

namespace std::chrono {
  ...
}
namespace std {
  ...
}
namespace std::chrono {
  ...
}
namespace std::inline literals::inline chrono_literals {
  ...
}
namespace std::chrono {
  using namespace literals::chrono_literals;
}

In addition to being more compact, this would mean we never have a (subtle) } appearing in the middle of the text switching us between namespaces. (In particular, the switch from std::chrono to std in the current synopsis is easy to miss, and an incautious reader might think that the inline namespace literals is in std::chrono, not in std.)

More generally, I think we should consider a policy that we never nest namespace definitions in the library clauses: names in namespace definitions should always be relative to the global namespace.
@tkoeppe
Copy link
Contributor

tkoeppe commented Sep 30, 2020

I welcome the general policy. I've started to find nested namespaces rather confusing as we've started having deeper nesting. I've even considered whether we should start applying } // namespace std-like close comments (as in some house styles) to aid orientation. The proposed policy alleviates this considerably.

@jensmaurer
Copy link
Member

jensmaurer commented Oct 1, 2020 •
edited

In [algorithms], we have a lot of

namespace std {
...
namespace ranges {
   ...
}
...
namespace ranges {
   ...
}
}

Closing namespace std in between would tear apart related non-ranges and ranges algorithms more than they deserve.

@jensmaurer jensmaurer added decision-required A decision of the editorial group (or the Project Editor) is required. big An issue causing a large set of changes, scattered across most of the text. labels Oct 2, 2020
@jensmaurer jensmaurer removed big An issue causing a large set of changes, scattered across most of the text. decision-required A decision of the editorial group (or the Project Editor) is required. labels Nov 19, 2021
@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 19, 2021

Given the usefulness of the compactness in <algorithms>, I'd like to prosose the following:

Richard's proposal is useful where it improves clarity, and in those cases we should feel welcome to use that style. In the algorithms case, the proposal would not improve clarity, and we can retain the status quo.

Ultimately, the goal is not some arbitrary consistency with some useless rule, but to make a clear and understandable document. So above all be consistent with "looks good".

For now, we will only adjust <chrono> to use Richard's proposed style. Any further changes would require case-by-case analysis.

@jensmaurer jensmaurer changed the title use nested namespace definitions in the library where applicable [chrono] Use nested namespace definitions in the library where applicable Nov 19, 2021
@jensmaurer jensmaurer self-assigned this Nov 19, 2021
@jensmaurer jensmaurer mentioned this issue Nov 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jensmaurer jensmaurer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[chrono.syn] Use nested namespace definitions for clarity jensmaurer/draft
3 participants
@zygoloid @tkoeppe @jensmaurer