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.

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

Enforced ES.23 upon example code: Introduction and Philosophy #1577

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Enforced ES.23 upon example code: Introduction and Philosophy #1577

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 29, 2020

I noticed much of the example code does not (yet) follow rule ES.23.
So I replaced occurrences of the form int x = 1; with the int x{ 1 }; form.

@hsutter hsutter self-assigned this Mar 19, 2020
@hsutter
Copy link
Contributor

hsutter commented Mar 19, 2020

Editors call: Thanks, you are correct that we are not consistent with ES.23 and we should be.

Note that ES.23 adds: "Use = only when you are sure that there can be no narrowing conversions. For built-in arithmetic types, use = only with auto."

So could you please change the PR to do the following:

  1. Auto: For every auto x = expr; there should be no change. For example, line 901 is already correct for ES.23.

  2. For loops: For for loop scope variables of integer type with a literal initializer (usually 0), change to auto and a u suffix on the initializer. For example, line 507 should change from for (int i = 0; to for (auto i = 0u;.

  3. Int: Wherever the current text explicitly writes int, use auto instead. For example, line 506 should change from int index = -1; to auto index = -1;.

@hsutter
Copy link
Contributor

hsutter commented Mar 19, 2020

Also note in line 927 there is a bug in the PR where we lost the identifier x.

@ghost
Copy link
Author

ghost commented Mar 20, 2020

Auto: For every auto x = expr; there should be no change.

Also note in line 927 there is a bug in the PR where we lost the identifier x.

It seems I went over it too hastily. Thanks for pointing these out.

@ghost
Copy link
Author

ghost commented Mar 26, 2020

Feel free to merge this PR, as I will leave it unchanged for now.

For built-in arithmetic types, use = only with auto

Should this not only apply when using a pre-17 standard? Which of course raises the question:
What standard should the example code follow?
Using C++17 - or a later standard - we could write for (auto i{ 0u };.

@hsutter
Copy link
Contributor

hsutter commented Mar 26, 2020

Editors call: Looks just about ready, thanks! One last minor thing please, especially in case you are planning to next do the same for other sections of the guidelines -- please use the int i {0}; style rather than int i{ 0 }; style. For example, line 975/976 is different only in that style and we prefer the first one. Would you be able to update it that way and then we can merge?

@JohelEGP
Copy link
Contributor

Shouldn't that be int i{0}?

@ghost
Copy link
Author

ghost commented Mar 26, 2020

Good luck.

@markyin
Copy link

markyin commented Mar 27, 2020

int i {0}; is better than int i{0} . :)

@ghost
Copy link
Author

ghost commented Mar 27, 2020

...but nothing beats int i{ 0 };
To better compare:

int i{ 7 };

int i {7};

int i{7};
type name{ value };

type name {value};

type name{value};
std::vector<int> v{ 7, a, d };

std::vector<int> v {7, a, d};

std::vector<int> v {7,a,d};

std::vector<int> v{7, a, d};

std::vector<int> v{7,a,d};

@Quuxplusone
Copy link
Contributor

https://quuxplusone.github.io/blog/2019/02/18/knightmare-of-initialization/#simple-guidelines-for-variable-i

In particular, please don't write for (auto i{ 0 }; i < 10; ++i) when you could write
for (int i=0; i < 10; ++i).

@alaestor
Copy link

alaestor commented Nov 8, 2023

It's a shame they seem to prefer i {0} because I find i{ 0 } much more aesthetically pleasing and easy to parse. i { 0 } is fine too, though it can look a bit too spacious in a minimal-character example like that, and I personally tend to favor visually acquiring the value first.

Having space around the value makes it easier to acquire at a glance because it has good visual blocking. Blocking is crucial to how most people perceive structure, from which we use derive visual meaning. Contextual color-coding in fancy editors can help, but structure is typically the primary mechanism which is why it's such an important aspect of visual design.

i = 0; has a very distinct signature (A a Aa). i=0; is okay at a glance due to typographical volume differences which help convey structure (AaAa). But i {0}; is slightly less obvious (A AAAa) and i{0}; would be the worst option available (AAAAa). Both i{ 0 }; (AA A Aa) and i { 0 }; (A A A Aa) are superior at being able to 'acquire' the rhs value. The former puts more emphasis on the rhs while the latter has equal emphasis on both lhs and rhs. It looks odd in small cases but it's a lot better when you have { more_letters }

Edit: the distinction is less pronounced here because the font on this page seems to distend the {}; glyphs further bellow alpha-numeric ones a{}; more than most other monospace and terminal fonts do.

Though it's only an anecdote, I've heard strong preference for { value }; opposed to {value}; from a few neurodivergent people. Personally, I find it much easier to acquire and parse at a glance, as it feels less busy? cluttered? noisy? claustrophobic? Also, whitespace after the { is visually invariant under many different circumstances; it maintains it's visual structure even in rare cases like a multi-line init (which I end up doing a lot when working with older code).

const T variable {
    []{
        T t;
        t.setup();
        return t;
    }()
};

But ultimately, it doesn't matter. The (local maximum) "best" style is the one that you can convince people to apply consistently...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants