<div dir="ltr">On Mon, Jun 24, 2013 at 12:22 PM, Tom Stellard <span dir="ltr"><<a href="mailto:tom@stellard.net" target="_blank">tom@stellard.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Ping.<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div style>Oops, sorry.</div><div style><br></div><div>+/// \p Operands [out] A map used to generate the OpName enum with operand</div><div>+///     names as its keys and operand enum values as its values.</div>
<div>+///  \p OperandMap [out] A map for representing the operand name mappings for</div><div>+///     each instructions.  This is used to generate the OperandMap table as</div><div style>+///     well as the getNamedOperandIdx() function. </div>
<div style><br></div><div style>These are aligned differently. Also, the prevailing style in the codebase seems to be `\param`.</div><div style><br></div><div style><div>+  for (unsigned i = 0, e = NumberedInstructions.size(), NumOperands = 0;</div>
<div>+                                                                  i != e; ++i) {</div><div><br></div><div style>Could you pull NumOperands onto its own line before the `for` like you did for `TableIndex` in another part of the code? I think that's a lot more readable since it doesn't hide an unrelated variable inside the `for` header.</div>
<div style><br></div><div style><div>+        const std::vector<const CodeGenInstruction *> NumberedInstructions,</div><div>+        std::string Namespace,</div><div><br></div><div>+  std::string Namespace = Target.getInstNamespace();<br>
</div><div><br></div><div style>Do you need to be making copies here?</div><div style><br></div><div style>+    for (unsigned i = 0, e = Operands.size(); i != e; ++i)<br></div><div style><br></div><div style>I think this will provoke a -Wshadow warning. Best to rename the nested loop variables like you did elsewhere (also, it would be nice if you were consistent in the naming of the nested loop counters (`j`, `je` vs. `ii`, `ie`)).</div>
<div style><br></div><div style>Other than that, it looks good to me. Jakob, any other thoughts?</div><div style><br></div><div style>-- Sean Silva</div></div></div></div></div></div>