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

Change Pack implementation to ensure constness of template arg is honored? #89

Open
bartgol opened this issue Mar 4, 2021 · 1 comment

Comments

@bartgol
Copy link
Contributor

bartgol commented Mar 4, 2021

The way the pack is implemented now, this code

  Pack<const Real,8> p(1.0);
  p[2] = 2.0;
  std::cout << "p:";
  for (int i=0; i<8; ++i) {
    std::cout << " " << p[i];
  }
  std::cout << "\n";

compiles, and prints

p: 1 1 2 1 1 1 1 1

The reason is that, internally, Pack only deals with a non-const version of ScalarType, delegating const-correctness to the constness of *this only. While that can be helpful in keeping code complexity under control (see below), it can also cause a bit of puzzlement in new devs.

The current behavior is handy in fcns like this:

template<typename T, int n>
Pack<T,n> square (const Pack<T,n>& p) {
  Pack<T,n> p2;
  for (int i=0; i<n; ++i)
      p2[i] = p[i]*p[i];
  return p2;
}

If T=const S, for some S, the return type is ok only because Pack<T,n> stores an array of std::remove_const<T>::type. Without this trick, the correct implementation of square would be


template<typename T, int n>
Pack<typename std::remove_const<T>::type,n> square (const Pack<T,n>& p) {
  Pack<typename std::remove_const<T>::type,n> p2;
  for (int i=0; i<n; ++i)
      p2[i] = p[i]*p[i];
  return p2;
}

which is arguably a bit more complex. It would, however, avoid to accidentally change the entries of a Pack<const T,n> pack...

Note: we could also have, inside the Pack struct declaration, something like:

  using non_const_type = Pack<typename std::remove_const<T>::type,n>;

which would turn the square signature into

template<typename T, int n>
typename Pack<T,n>::non_const_type square(const Pack<T,n>& p);

Not a huge improvement, but still smaller, and perhaps more verbose.

tl;dr Allowing a Pack<const double, 8> to be modified is counter-intuitive at best, and dangerous at worst. We should avoid bugs down the road due to this.

@bartgol
Copy link
Contributor Author

bartgol commented Mar 4, 2021

Note: in order to allow initialization with a for loop (i.e., do the v[i]=... inside the ctor body), Pack<T,n> should still store an array of non-const scalars. What really matters is that it should have the op[] signature changed, like so:

T& operator[] (int i);
typename std::add_const<T>::type& operator[] (int i) const;

Notice that the two return types might as well be both const (if T is itself a const type).

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

No branches or pull requests

1 participant