Ah, I didn't realize the current wording of the *auto* use cases were that strict. But let me explain, and see what you and others think.
First off, we already have uses of *const auto* and *const **auto&* in the codebase. From my reading, it seems that you feel the same way about them as you do towards *auto** (correct me if I'm reading wrong). I think The deduction rules for *auto* are equivalent to template argument deduction rules. One must know and understand that fact in order to use *auto* correctly. Given *auto x = expr;* one may "intuitively" think that *auto *simply takes on the exact type of the right hand side, which is not what happens (we have *decltype(auto)* for that). So I don't agree that *auto&* and *auto** are any less intuitive than *auto*. IMO, it follows that the guideline for the qualifiers surrounding *auto* is equivalent to the guideline for qualifiers surrounding a template parameter. * template <typename T> void F(T x);* *F(expr); *an argument of arbitrary type by value, *auto x = expr;* * template <typename T> void F(const T& x); F(expr);* an argument of arbitrary type by const-ref *const auto& x = expr;* * template <typename T> void F(const T* x); F(expr);* an argument of arbitrary type, const-pointer *const auto* x = expr;* ... In the case being considered, we could have simply used *auto* because a decaying a pointer has no effect (I don't think this is obvious). * auto ptr = static_cast<const T*>(&t);* // *const T** decayed is *const T**, so *ptr* is *const T** and we're ok. However, consider how a seemingly innocent modification can become incorrect with an assumption that *auto* simply takes the exact type of the right hand side: *auto ref = static_cast<const T&>(t);* // incorrect assumption: type of *ref* is *const T&* *const T* ptr = &ref;* // incorrect assumption: *ptr == &t* We actually need: *const auto& ref = static_cast<const T&>(t);* in order for *ptr == &t* to be true. With that in mind, I would prefer to write the above code as: *const auto* ptr = static_cast<const T*>(&t);* and the innocent-looking modification as: *const auto& ref = static_cast<const T&>(t);* * const T* ptr = &ref;* Thanks, MPark. On Thu, Dec 3, 2015 at 10:20 PM Benjamin Mahler <[email protected]> wrote: > Why did you guys choose to use 'auto*' here? These are the first instances > in the code base, can we remove them? Let's consider 'auto&' and 'auto*' > and update the style guide first, IMHO 'auto*' and 'auto&' are less > intuitive, so we may want to be more conservative about them in general. > > On Thu, Dec 3, 2015 at 12:39 AM, <[email protected]> wrote: > > > Repository: mesos > > Updated Branches: > > refs/heads/master 0a82c2b12 -> 49d3cb112 > > > > > > Fixed pointer alignment error in IP::create(). > > > > The previous code took the address of a `struct sockaddr`, and then cast > > the > > resulting pointer to `struct sockaddr_storage *`. The alignment > > requirements > > for `struct sockaddr_storage *` are more strict than for `struct sockaddr > > *`, > > and hence this code produced undefined behavior per ubsan in GCC 5.2. > > > > Drive-by Fix: Updated the code to use `reinterpret_cast` rather than a > > C-style > > cast, and to preserve constness. > > > > Review: https://reviews.apache.org/r/40435 > > > > > > Project: http://git-wip-us.apache.org/repos/asf/mesos/repo > > Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/49d3cb11 > > Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/49d3cb11 > > Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/49d3cb11 > > > > Branch: refs/heads/master > > Commit: 49d3cb1125cfcc8644e7f37b8d8654729aecd657 > > Parents: 0a82c2b > > Author: Neil Conway <[email protected]> > > Authored: Thu Dec 3 03:13:22 2015 -0500 > > Committer: Michael Park <[email protected]> > > Committed: Thu Dec 3 03:21:50 2015 -0500 > > > > ---------------------------------------------------------------------- > > .../3rdparty/stout/include/stout/ip.hpp | 39 > ++++++++++++++------ > > 1 file changed, 27 insertions(+), 12 deletions(-) > > ---------------------------------------------------------------------- > > > > > > > > > http://git-wip-us.apache.org/repos/asf/mesos/blob/49d3cb11/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > ---------------------------------------------------------------------- > > diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > b/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > index 3e506e1..1d34d4e 100644 > > --- a/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp > > @@ -211,23 +211,38 @@ inline Try<IP> IP::parse(const std::string& value, > > int family) > > > > inline Try<IP> IP::create(const struct sockaddr_storage& _storage) > > { > > - switch (_storage.ss_family) { > > - case AF_INET: { > > - struct sockaddr_in addr = *((struct sockaddr_in*) &_storage); > > - return IP(addr.sin_addr); > > - } > > - default: { > > - return Error( > > - "Unsupported family type: " + > > - stringify(_storage.ss_family)); > > - } > > - } > > + // According to POSIX: (IEEE Std 1003.1, 2004) > > + // > > + // (1) `sockaddr_storage` is "aligned at an appropriate boundary so > that > > + // pointers to it can be cast as pointers to protocol-specific address > > + // structures and used to access the fields of those structures > without > > + // alignment problems." > > + // > > + // (2) "When a `sockaddr_storage` structure is cast as a `sockaddr` > > + // structure, the `ss_family` field of the `sockaddr_storage` > structure > > + // shall map onto the `sa_family` field of the `sockaddr` structure." > > + // > > + // Therefore, casting from `const sockaddr_storage*` to `const > > sockaddr*` > > + // (then subsequently dereferencing the `const sockaddr*`) should be > > safe. > > + // Note that casting in the reverse direction (`const sockaddr*` to > > + // `const sockaddr_storage*`) would NOT be safe, since the former > might > > + // not be aligned appropriately. > > + const auto* addr = reinterpret_cast<const struct > sockaddr*>(&_storage); > > + return create(*addr); > > } > > > > > > inline Try<IP> IP::create(const struct sockaddr& _storage) > > { > > - return create(*((struct sockaddr_storage*) &_storage)); > > + switch (_storage.sa_family) { > > + case AF_INET: { > > + const auto* addr = reinterpret_cast<const struct > > sockaddr_in*>(&_storage); > > + return IP(addr->sin_addr); > > + } > > + default: { > > + return Error("Unsupported family type: " + > > stringify(_storage.sa_family)); > > + } > > + } > > } > > > > > > > > >
