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

Should ES.23 mention that ()-initialization is likely to be preferrable when dealing with containers containing types initializable from the container itself #2205

Open
Aster89 opened this issue Jun 12, 2024 · 7 comments

Comments

@Aster89
Copy link

Aster89 commented Jun 12, 2024

Here's ES.23.

Should that mention that one might want to prefer ()-initialization to {}-initialization when initializing a container of elements of a given type from a container of the same type, when the inner type can be constructed from the vector itself?

I'm referring to the fact that

std::vector<std::any> v{1,2,3};
std::vector<std::any> w{v};

will result in w having length 1, with its only element v[0] having been initialized from v. I think this can be surprising to most programmers.

A similar scenario happens when, instead of std::any, the type inside the container has a templated constructor, like in this case.

@iglesias
Copy link
Contributor

Should that mention that one might want to prefer ()-initialization to {}-initialization when initializing a container of elements of a given type from a container of the same type, _when the inner type can be constructed from the vector itself?

I am not sure about that.

I'm referring to the fact that

std::vector<std::any> v{1,2,3};
std::vector<std::any> w{v};

will result in w having length 1, with its only element v[0] having been initialized from v. I think this can be surprising to most programmers.

I think w[0] was meant in place of v[0].
What do you think wouldn't surprise most programmers in this case?
In my opinion based on looking at it now, it looks like an artificially convoluted piece of code (in order to define a - single-element - vector, of vector(s), of anys), but I think it looks like the expected behavior.

@Aster89
Copy link
Author

Aster89 commented Jul 21, 2024

I think w[0] was meant in place of v[0].

Yes, sorry.

What do you think wouldn't surprise most programmers in this case?

On the contrary, I think that what happens with std::vector<std::any> w{v}; would surprise most programmers at first.

What I'm saying is that it would beneficial if ES.23 mentioned that there are cases where preferring {}-initializer syntax results in not very easy to debug bugs.

it looks like an artificially convoluted piece of code

Ok, to be more concrete, I had to update a class that looked like this,

struct Foo {
  Foo(v, /* other args */)
    : v{}
    , /* other init.ons */
  std::vector<std::any> v;
  // other member vars + API
};

where v would then be altered via some API. This all worked fine. the {} instead of () in v{} was out of the habit of abiding by ES.23.

Later, the need arised for v to be initialized from some argument:

struct Foo {
  Foo(v, /* other args */)
    : v{std::move(v)}
    , /* other init.ons */
  std::vector<std::any> v;
  // other member vars and member funcs
};

But this code is broken for the reason I explained, and it's not easy to debug because you just start seeing std::any_cast fail and you don't immediately understand (I'm sure I'm passing a std::vector<Bar> to Foo's ctor, so why can't I cast an element of v to Bar?).


Another real-life case. Take this talk from Sean Parent about Runtime Polymorphism, where he always uses ()-initialization instead of {}-initialization. When I tried to rewrite that code myself, I came up with this, which is fundamentally his code, but with {} instead of () for initalizing things, becuase that's my habit, i.e. follow ES.23.

The result is that the code is bugged on lines 11-12, and the behavior is a neverending recursive call that stackoverflows.

Making the ctor explicit would have prevented the problem in the first place. But without that, it's clear that the {} is not really resulting in what one generally expects if they write self{std::make_unique<model<T>>(std::move(t))}.

@shaneasd
Copy link
Contributor

The same point is made in https://abseil.io/tips/88

@iglesias
Copy link
Contributor

The same point is made in https://abseil.io/tips/88

"use the traditional constructor syntax when initialization is performing some active logic".

@jwakely
Copy link
Contributor

jwakely commented Jul 22, 2024

In my opinion based on looking at it now, it looks like an artificially convoluted piece of code (in order to define a - single-element - vector, of vector(s), of anys), but I think it looks like the expected behavior.

I assure you that people write code like this all the time, and are surprised by it.

I think they're right to be surprised that T a; T b{a}; does not make a copy of a.

It's also surprising that compilers don't agree on this behaviour, because some refuse to implement what the standard says here.

@iglesias
Copy link
Contributor

Thank you very much for the comments.

I think they're right to be surprised that T a; T b{a}; does not make a copy of a.

I hadn't considered the second vector didn't make its own copy of the first one. In that case, the data would be shared, if I am interpreting it and then deducing correctly, and it got me pondering. I have been experimenting trying to reproduce but haven't managed so far, this is what I get:

#include <any>
#include <iostream>
#include <vector>

using std::any;
using std::any_cast;
using std::vector;

int main()
{
    std::vector<std::any> v{1,2,3};
    v[1] = 9;
    std::vector<std::any> w{v};

    std::cout << "w before reset:\n  "
              << w.size() << ' '
              << w[0].has_value() << ' '
              << std::any_cast<std::vector<std::any>>(w[0]).size() << '\n' << "  ";

    //std::any_cast<std::vector<std::any>>(w[0])[1] = 7;  // puzzling that the assignment doesn't seem to modify "w[0][1]"
    v[1] = 7;                                            

    std::cout << any_cast<int>(any_cast<vector<any>>(w[0]).at(0)) << ' '
              << any_cast<int>(any_cast<vector<any>>(w[0]).at(1)) << ' '
              << any_cast<int>(any_cast<vector<any>>(w[0]).at(2)) << '\n';
    
    w[0].reset();

    std::cout << "w after reset:\n  "
              << w.size() << ' '
              << w[0].has_value() << '\n';
    //std::cout << std::any_cast<std::vector<std::any>>(w[0]).size() << '\n';  // compilation error, OK, it must cos of reset
    
    std::cout << "v:\n  "
              << v.size() << ' '
              << v[0].has_value() << ' '
              << v[1].has_value() << ' '
              << v[2].has_value() << '\n' << "    ";

    std::cout << any_cast<int>(v[0]) << ' '
              << any_cast<int>(v[1]) << ' '
              << any_cast<int>(v[2]) << '\n';
}
w before reset:
  1 1 3
  1 9 3
w after reset:
  1 0
v:
  3 1 1 1
    1 7 3

https://godbolt.org/z/ras68s6n6

From the result, I think that there is a copy: before the reset, the middle element in w is 9 and after the reset v still contains valid data and the middle element is 7. Perhaps is there an explanation related to the other comment on compilers and the standard? Interesting. I will continue taking a look and see what I figure out.

@jwakely
Copy link
Contributor

jwakely commented Jul 24, 2024

I think you misunderstood. Obviously there's no data sharing between the vectors, that's not the point.

My point is that for most types, T a; T b{a}; creates b as a copy of a. The result is that b is a new object equal to a.

But in the examples above, it makes a copy of a and then converts that to std::any and then inserts that into b. That is not the same thing. Not even close. The new object b is not a copy of a. It contains a copy of a.

Aster89 added a commit to Aster89/CppCoreGuidelines that referenced this issue Sep 10, 2024
Addressing the issue isocpp#2205 I opened myself.

I can't see why not contributing.
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

No branches or pull requests

4 participants