Sorry, the first example from protobuf_utils.cpp isn't so bad, although we're inconsistent with how we name iterators. Also, it would be great to use foreach with a reversed adaptor rather than manual reverse iteration.
On Fri, Dec 4, 2015 at 12:01 PM, Benjamin Mahler <[email protected]> wrote: > (Hm.. were you using bold formatting here? Apache's mail servers will > strip it and change it into *asterisks*.) > > Yes, in general we want to be conservative with auto, and a lot of > additions got through review that should not have. For example: > > src/common/protobuf_utils.cpp: for (auto status = > task.statuses().rbegin(); > src/local/local.cpp: auto authorizerNames = > strings::split(flags.authorizers, ","); > src/master/http.cpp: auto ids = > ::protobuf::parse<RepeatedPtrField<MachineID>>(jsonIds.get()); > src/slave/containerizer/mesos/provisioner/docker/token_manager.cpp: > const auto padding = segment.length() % 4; > src/tests/fault_tolerance_tests.cpp: auto capabilityType = > FrameworkInfo::Capability::REVOCABLE_RESOURCES; > > These definitely do not fit with our style guidelines around improving > readability. The potential traps you've described are even more reason to > be conservative with auto usage in our project. > > In many of these cases, the type is not obvious from the right-hand-side. > If the person reading the code has to think in order to figure out what > 'auto' is, we shouldn't be using it. My big concern here is that we need to > make it as easy as possible to read and understand the code. > > Also, if the type is not onerous, why be aggressive with 'auto'? > > On Fri, Dec 4, 2015 at 1:10 AM, Michael Park <[email protected]> wrote: > >> 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)); >> > > + } >> > > + } >> > > } >> > > >> > > >> > > >> > > >> > >> > >
