[PATCH] D14961: LLVM CodeView library

[Sean Silva via llvm-commits]

silvas added a subscriber: silvas.
silvas added a comment.

A couple style nits.


================
Comment at: include/llvm/DebugInfo/CodeView/CodeView.h:122
@@ +121,3 @@
+
+inline FunctionOptions operator|(FunctionOptions a, FunctionOptions b) throw() {
+  return static_cast<FunctionOptions>(static_cast<uint8_t>(a) |
----------------
In LLVM we usually don't use `throw()`. Unless there is a specific reason you're using it (in which case please put a prominent comment somewhere explaining the reason), please remove it.

================
Comment at: lib/DebugInfo/CodeView/CodeViewOStream.cpp:11
@@ +10,3 @@
+#include "llvm/DebugInfo/CodeView/CodeViewOStream.h"
+
+using namespace llvm;
----------------
This file also seems unneeded.

================
Comment at: lib/DebugInfo/CodeView/FunctionId.cpp:13
@@ +12,3 @@
+namespace llvm {
+namespace codeview {}
+}
----------------
What is the point of this file? Can you just delete it for now?

================
Comment at: lib/DebugInfo/CodeView/ListRecordBuilder.cpp:28
@@ +27,3 @@
+
+  // TODO: Continuations
+  assert(Builder.size() < 65536);
----------------
Is this TODO related to the assertion?

================
Comment at: lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp:13
@@ +12,3 @@
+
+namespace llvm {
+namespace codeview {
----------------
In LLVM, we typically put a `using namespace llvm` (and any other namespaces) at the top of the .cpp file instead of wrapping the whole file in namespaces.

================
Comment at: lib/DebugInfo/CodeView/TypeIndex.cpp:11
@@ +10,3 @@
+#include "llvm/DebugInfo/CodeView/TypeIndex.h"
+
+using namespace llvm;
----------------
This file also seem to be unneeded.

================
Comment at: lib/DebugInfo/CodeView/TypeTableBuilder.cpp:195
@@ +194,3 @@
+       SlotIndex += 2) {
+    uint8_t Byte = static_cast<uint8_t>(Record.getSlots()[SlotIndex]) << 4;
+    if ((SlotIndex + 1) < Record.getSlots().size()) {
----------------
Pull `Record.getSlots()` out into a local variable. That will shorten/clarify a lot of this code.

================
Comment at: lib/DebugInfo/CodeView/TypeTableBuilder.cpp:210
@@ +209,3 @@
+TypeIndex TypeTableBuilder::writeFieldList(FieldListRecordBuilder &FieldList) {
+  // TODO: Continuations
+  return writeRecord(FieldList.str());
----------------
Please write complete sentences http://llvm.org/docs/CodingStandards.html#commenting


http://reviews.llvm.org/D14961