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

Editorial: Make names and synopses of unordered_* consistent with other containers #400

Closed
tkoeppe opened this issue Nov 11, 2014 · 6 comments
Closed

Editorial: Make names and synopses of unordered_* consistent with other containers #400

tkoeppe opened this issue Nov 11, 2014 · 6 comments
Assignees
@jwakely

Comments

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 11, 2014

  • The synopses for unordered_* don't contain sections before comments like // modifiers, whereas the other container synopses do.
  • Unordered containers use k for key parameters, ordered associative containers use x.
  • Unordered containers use hint for the hint iterator, other containers use position.

We should make this consistent.

Suggestion:

  • Use hint for hints, position for everything else. This requires changing the ordered containers.
  • Use k for the key, and obj for a parameter for the value_type or the mapped_type. This requires changing the ordered containers.
  • Always refer to the section name in comments. This requires changing the unordered containers.

If we agree with these changes, I can create a suitable pull request. Please let me know.
@zygoloid
Copy link
Member

SGTM

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Nov 15, 2014

I'll wait for the motions to be merged, since this cleanup will affect some of them.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Dec 14, 2014

See commit 3dd6142 in my fork. For some reason GitHub doesn't display the diff.

@zygoloid
Copy link
Member

Diff is visible here: https://github.com/cplusplus/draft/commit/3dd61428d27a091bd16d833598805f0a93bbb319.patch

@jwakely Can I interest you in reviewing this?

@jwakely
Copy link
Member

jwakely commented Feb 23, 2015

The first 18% of the patch looks perfect, but for the purposes of reviewing it, and also for the revision history ("what introduced this change?" "oh, it was that 3000 line patch that fixed loads of unrelated issues") I'd like it to be split into logically separate changes that I can review and merge piecemeal.

tkoeppe added a commit to tkoeppe/draft that referenced this issue Mar 22, 2015
@tkoeppe
Consistency cleanups in [containers] (cf. Issue cplusplus#400). This …
95a6dc1 
…part of the cleanup changes only whitespace: inserted before commas, removed after namespace braces, improved indentation consistency, removed some extraneous linebreaks for things that fit well on one line
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Mar 22, 2015

Offline discussion suggested that these changes better be split into several smaller PRs. Please consider the original diff obsolete, and I will send a number of new PRs.

@tkoeppe tkoeppe mentioned this issue Mar 22, 2015
Merged
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 2, 2015
@tkoeppe
Consistency cleanups in [containers] (cf. Issue cplusplus#400). This …
ee71f5d 
…part of the cleanup changes only whitespace: inserted before commas, removed after namespace braces, improved indentation consistency, removed some extraneous linebreaks for things that fit well on one line
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 2, 2015
@tkoeppe
Consistency cleanups in [containers] (cf. Issue cplusplus#400). This …
99c10db 
…part of the cleanup changes only whitespace: inserted before commas, removed after namespace braces, improved indentation consistency, removed some extraneous linebreaks for things that fit well on one line
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 14, 2015
@tkoeppe
Consistecy cleanups in [containers] (cf. Issue cplusplus#400): Orderi…
91315f5 
…ng of members, whitespace
@tkoeppe tkoeppe mentioned this issue Apr 14, 2015
Merged
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 14, 2015
@tkoeppe
Consistecy cleanups in [containers] (cf. Issue cplusplus#400): Orderi…
f1b8280 
…ng of members, whitespace
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 14, 2015
@tkoeppe
[containers] Consistecy cleanups (cf. Issue cplusplus#400): Ordering …
24cf1c3 
…of members, whitespace
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 14, 2015
@tkoeppe
[containers] Consistecy cleanups (cf. Issue cplusplus#400): Ordering …
78c017c 
…of members, whitespace
tkoeppe added a commit to tkoeppe/draft that referenced this issue Apr 14, 2015
@tkoeppe
[containers] Consistency cleanups (cf. Issue cplusplus#400): Ordering…
248c164 
… of members, whitespace
jwakely added a commit that referenced this issue Apr 14, 2015
@jwakely
Merge pull request #477 from tkoeppe/consistency_fixes_2
94ba047 
Consistency cleanups in [containers] (cf. Issue #400): Ordering, whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jwakely jwakely

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@zygoloid @jwakely @tkoeppe @jensmaurer