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

[Adrian McCarthy via llvm-commits]

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?

> (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.

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.

================
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.

================
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.)

> 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