[PATCH] D19746: NFC: An iterator for stepping through CodeView type stream in llvm-readobj

Mon May 2 12:32:55 PDT 2016

On Mon, May 2, 2016 at 11:46 AM, Adrian McCarthy via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> amccarth marked 8 inline comments as done.
> amccarth added a comment.
>
> Thanks for all the detailed feedback.  I'll take a stab at a named
> constructor to fix the awkwardness of handling the magic number before I
> update the diff.
>
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:1981-1982
> @@ +1980,4 @@
> +    const std::error_code Error = Section.getContents(Data);
> +    if (Error)
> +      throw Error;
> +    if (Data.size() >= 4) {
> ----------------
> dblaikie wrote:
> > You could roll the variable declaration into the conditional to reduce
> its scope to just as much as is needed:
> >
> >   if (std::error_code Error = ...)
> >     throw Error;
> >
> > Also - throw? LLVM doesn't use exceptions...
> >
> > You may need a named ctor, or another way of handling construction
> failure.
> Sorry about that.  I meant to circle back and take care of the throw.
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:1994
> @@ +1993,3 @@
> +  // in the same data stream, or they must both be at the end of a stream.
> +  bool operator==(const CodeViewTypeIterator &other) const {
> +    return (Data.begin() == other.Data.begin()) || (AtEnd && other.AtEnd);
> ----------------
> dblaikie wrote:
> > Generally any operator overloads that can be non-members, should be
> (ensures that any implicit conversions are equally viable to the LHS and
> RHS of an expression using that operator)
> >
> > You can just stick "friend" in front of these, and add the other
> parameter to the parameter list rather than using 'this'.
> > Generally any operator overloads that can be non-members, should be
>
> Yes, but I generally interpret "can be" as "implementable via the rest of
> its public interface."  With `friend` you can always make it a non-member,
> right?
>

Sorry, in my sense I meant "can be" to be "the language allows it" (some
operators cannot be implemented as non-members - such as the function call
operator (I'm not sure what the complete list is)).

> > (ensures that any implicit conversions are equally viable to the LHS and
> RHS of an expression using that operator)
>
> Right, but this class has no implicit conversions.
>

Right - but other types could have implicit conversions to this type.
("operator X ()" is an implicit conversion to X)

> I'm happy to make the change if you think it's worthwhile.  I'm just
> explaining why I didn't do it that way the first time.
>

Yeah, for sure - I'd encourage it just out of good
habits/examples/consistency (consistency with best practices, not
consistency with LLVM - which isn't consistent on this at all). Your call,
though.

>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:2002
> @@ +2001,3 @@
> +
> +  unsigned getMagic() const { return Magic; }
> +
> ----------------
> dblaikie wrote:
> > Having a custom function on an iterator like this might be a bit awkward
> (eg: you wouldn't be able to access this value in a range-based for loop).
> Is that OK?
> >
> > Perhaps this could be addressed along with the "how to fail in the ctor"
> issue - or, maybe if the ctor has to be fail-able anyway, you wouldn't just
> build one in a range-based-for anyway, so calling a member function on this
> iterator wouldn't be too surprising.
> >
> > On the 3rd hand - Magic doesn't seem to be used by this iterator, it's
> just computed during construction - so once there's a named ctor thing that
> can produce an error, it can also produce the Magic without it being part
> of the iterator (& thus reducing the size of the iterator making it more
> suitable to do iterator-y things with (pass by value, copy cheaply, etc))
> I agree it's awkward.  This wrapper didn't start out with an iterator-like
> interface.  The intent was to hide the details of parsing the "stream."
> The "stream" consists of a magic number followed by a series of types.
> Eventually the iterator interface began to make sense for everything after
> the magic number.
>
> The caller may or may not care about the magic number.  The llvm-readobj
> does, but another client might not, and it would be perfectly happy to use
> a range-based for loop to just get the types.  This allows both approaches.
>
> Let me consider the named constructor.
>

Yeah, happy to throw around a few ideas here.

Having the named ctor return ErrorOr<TypeStream> where TypeStream is just
an iterator range+magic (so it has begin/end/getMagic)?

>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:2020
> @@ +2019,3 @@
> +  CodeViewTypeIterator operator++(int) {
> +    CodeViewTypeIterator original(*this);
> +    this->operator++();
> ----------------
> dblaikie wrote:
> > Prefer copy init over direct init when the two are equivalent. This
> avoids any accidental explicit conversions, making the code easier to read:
> >
> >   CodeViewTypeIterator original = *this;
> >
> > Also, naming convention - variables start with upper case (
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> )
> > [Copy initialization] avoids any accidental explicit conversions
>
> Explicit conversions?  How so?  Both ways are prone to implicit
> conversions, which seems like more of a problem.  Fortunately, this class
> has no conversions.
>
> (I've made the change to be consistent with LLVM style.  I'm just trying
> to understand how direct init could allow an accidental explicit
> conversion.)
>

The best example is with unique_ptr:

  std::unique_ptr<T> x(y); // this can trigger explicit conversions from y
to unique_ptr<T>
  std::unique_ptr<T> x = y; // this can only trigger implicit conversions

Importantly, in the latter, 'y' cannot be a raw pointer. So when I'm
reading this code the second form is easier for me to read because I know
it's "safer"/less interesting. If I see the first form I may look more
closely to understand what explicit conversion might be occurring.

> > variables start with upper case
>
> Oops, I forgot.  That's going to be crazy hard to get used to.
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:2026
> @@ +2025,3 @@
> +private:
> +  void Next() {
> +    assert(!AtEnd && "Attempted to advance more than one past the last
> rec");
> ----------------
> rnk wrote:
> > In LLVM, methods are leading lower-case:
> >
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> >
> > We are famously inconsistent here and the rules don't make a lot of
> sense, but we're trying to improve.
> It's like living in Opposite Land. (-:
>
> ================
> Comment at: tools/llvm-readobj/COFFDumper.cpp:2035-2037
> @@ +2034,5 @@
> +    const TypeRecordPrefix *Rec;
> +    std::error_code Error = consumeObject(Data, Rec);
> +    if (Error)
> +      return;
> +    Current.Length = Rec->Len;
> ----------------
> dblaikie wrote:
> > Roll the variable declaration into the condition:
> >
> >   if (std::error_code Error = ...)
> >     return;
> >
> > & in fact, you'll get an unused variable warning then, so it'll just be:
> >
> >   if (consumeObject(...))
> >     return;
> Sorry, this was a leftover from when `Next` (now `next`) was returning the
> error.
>
>
> http://reviews.llvm.org/D19746
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160502/b4ff2369/attachment.html>