<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 2, 2016 at 11:46 AM, Adrian McCarthy via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">amccarth marked 8 inline comments as done.<br>
amccarth added a comment.<br>
<br>
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.<br>
<span class=""><br>
<br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:1981-1982<br>
@@ +1980,4 @@<br>
</span><span class="">+ const std::error_code Error = Section.getContents(Data);<br>
+ if (Error)<br>
+ throw Error;<br>
+ if (Data.size() >= 4) {<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> You could roll the variable declaration into the conditional to reduce its scope to just as much as is needed:<br>
><br>
> if (std::error_code Error = ...)<br>
> throw Error;<br>
><br>
> Also - throw? LLVM doesn't use exceptions...<br>
><br>
> You may need a named ctor, or another way of handling construction failure.<br>
</span>Sorry about that. I meant to circle back and take care of the throw.<br>
<span class=""><br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:1994<br>
@@ +1993,3 @@<br>
+ // in the same data stream, or they must both be at the end of a stream.<br>
+ bool operator==(const CodeViewTypeIterator &other) const {<br>
+ return (Data.begin() == other.Data.begin()) || (AtEnd && other.AtEnd);<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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)<br>
><br>
> You can just stick "friend" in front of these, and add the other parameter to the parameter list rather than using 'this'.<br>
> Generally any operator overloads that can be non-members, should be<br>
<br>
</span>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?<br></blockquote><div><br></div><div>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)).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> (ensures that any implicit conversions are equally viable to the LHS and RHS of an expression using that operator)<br>
<br>
</span>Right, but this class has no implicit conversions.<br></blockquote><div><br></div><div>Right - but other types could have implicit conversions to this type. ("operator X ()" is an implicit conversion to X)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:2002<br>
@@ +2001,3 @@<br>
+<br>
+ unsigned getMagic() const { return Magic; }<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> 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?<br>
><br>
> 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.<br>
><br>
> 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))<br>
</span>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.<br>
<br>
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.<br>
<br>
Let me consider the named constructor.<br></blockquote><div><br></div><div>Yeah, happy to throw around a few ideas here.<br><br>Having the named ctor return ErrorOr<TypeStream> where TypeStream is just an iterator range+magic (so it has begin/end/getMagic)?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:2020<br>
@@ +2019,3 @@<br>
+ CodeViewTypeIterator operator++(int) {<br>
+ CodeViewTypeIterator original(*this);<br>
+ this->operator++();<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Prefer copy init over direct init when the two are equivalent. This avoids any accidental explicit conversions, making the code easier to read:<br>
><br>
> CodeViewTypeIterator original = *this;<br>
><br>
> Also, naming convention - variables start with upper case ( <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" rel="noreferrer" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a> )<br>
</span>> [Copy initialization] avoids any accidental explicit conversions<br>
<br>
Explicit conversions? How so? Both ways are prone to implicit conversions, which seems like more of a problem. Fortunately, this class has no conversions.<br>
<br>
(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.)<br></blockquote><div><br></div><div>The best example is with unique_ptr:<br><br> std::unique_ptr<T> x(y); // this can trigger explicit conversions from y to unique_ptr<T><br> std::unique_ptr<T> x = y; // this can only trigger implicit conversions<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> variables start with upper case<br>
<br>
</span>Oops, I forgot. That's going to be crazy hard to get used to.<br>
<span class=""><br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:2026<br>
@@ +2025,3 @@<br>
+private:<br>
+ void Next() {<br>
+ assert(!AtEnd && "Attempted to advance more than one past the last rec");<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> In LLVM, methods are leading lower-case:<br>
> <a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly" rel="noreferrer" target="_blank">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a><br>
><br>
> We are famously inconsistent here and the rules don't make a lot of sense, but we're trying to improve.<br>
</span>It's like living in Opposite Land. (-:<br>
<span class=""><br>
================<br>
Comment at: tools/llvm-readobj/COFFDumper.cpp:2035-2037<br>
@@ +2034,5 @@<br>
+ const TypeRecordPrefix *Rec;<br>
+ std::error_code Error = consumeObject(Data, Rec);<br>
+ if (Error)<br>
+ return;<br>
+ Current.Length = Rec->Len;<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Roll the variable declaration into the condition:<br>
><br>
> if (std::error_code Error = ...)<br>
> return;<br>
><br>
> & in fact, you'll get an unused variable warning then, so it'll just be:<br>
><br>
> if (consumeObject(...))<br>
> return;<br>
</span>Sorry, this was a leftover from when `Next` (now `next`) was returning the error.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D19746" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19746</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>