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

[David Blaikie via llvm-commits]

dblaikie added a subscriber: dblaikie.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1855
@@ -1864,3 +1854,3 @@
   StringRef Leading, Trailing;
-  std::tie(Leading, Trailing) = Remaining.split('\0');
+  std::tie(Leading, Trailing) = LeafData.split('\0');
   return Leading;
----------------
You can use std::ignore if you don't want some of the results (eg: "std::tie(Leading, std::ignore)")

Or you could just write "return LeafData.split('\0').first;"

But this code was in the original, so not really your responsibility to refactor.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1979
@@ +1978,3 @@
+  explicit CodeViewTypeIterator(const SectionRef &Section)
+      : Data(), Magic(0), Current(), AtEnd(false) {
+    const std::error_code Error = Section.getContents(Data);
----------------
You can remove "Data()" from the init list, it doesn't do anything, presumably "Current()" too.

& you can use a non-static data member initializer for Magic. Then you just need to initialize AtEnd in the init list of the two ctors.

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:1980
@@ +1979,3 @@
+      : Data(), Magic(0), Current(), AtEnd(false) {
+    const std::error_code Error = Section.getContents(Data);
+    if (Error)
----------------
LLVM doesn't generally use const on local values (you can, just saying - it's a bit uncommon/atypical)

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

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

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:2002
@@ +2001,3 @@
+
+  unsigned getMagic() const { return Magic; }
+
----------------
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))

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:2020
@@ +2019,3 @@
+  CodeViewTypeIterator operator++(int) {
+    CodeViewTypeIterator original(*this);
+    this->operator++();
----------------
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 )

================
Comment at: tools/llvm-readobj/COFFDumper.cpp:2021
@@ +2020,3 @@
+    CodeViewTypeIterator original(*this);
+    this->operator++();
+    return original;
----------------
You could write this as ++*this;

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


http://reviews.llvm.org/D19746