<p dir="ltr"><br>
On May 23, 2016 10:54 PM, "Nirav Dave" <<a href="mailto:niravd@google.com">niravd@google.com</a>> wrote:<br>
><br>
> niravd added inline comments.<br>
><br>
> ================<br>
> Comment at: include/llvm/MC/MCStreamer.h:591<br>
> @@ +590,3 @@<br>
> +  /// \param Loc - The location of the expression for error reporting.<br>
> +  virtual void emitSpace(const MCExpr *Size, uint64_t FillValue,<br>
> +                         SMLoc Loc = SMLoc());<br>
> ----------------<br>
> Please rename to match casing of other Emit functions. (EmitSpace).<br></p>
<p dir="ltr">No. Use the new stile or we will never be done moving to it. If you want consistency, rename the existing ones and then rebase.</p>
<p dir="ltr">> ================<br>
> Comment at: include/llvm/MC/MCStreamer.h:602<br>
> @@ +601,3 @@<br>
> +  /// \param Loc - The location of the expression for error reporting.<br>
> +  virtual void emitRepeatedValue(const MCExpr *NumValues, int64_t Size,<br>
> +                                 int64_t Expr, SMLoc Loc = SMLoc());<br>
> ----------------<br>
> Why isn't this named EmitFill? It's of a distinct type from the other calls and is only for dealing with Fill directives and the distinction seems more confusing than multiple variants of EmitFill.<br>
><br>
> ================<br>
> Comment at: include/llvm/MC/MCStreamer.h:605<br>
> @@ +604,3 @@<br>
> +<br>
> +  void emitRepeatedValueImpl(int64_t Repeat, int64_t Size, int64_t Value,<br>
> +                             SMLoc Loc = SMLoc());<br>
> ----------------<br>
> Save as above.<br>
><br>
> ================<br>
> Comment at: lib/MC/MCAsmStreamer.cpp:828<br>
> @@ +827,3 @@<br>
> +<br>
> +  OS << "\t.space\t";<br>
> +  NumBytes->print(OS, MAI);<br>
> ----------------<br>
> Since we cannot guarantee a lack of errors at this point we should emit line directives before and after this so that we preserve the error message location with respect to the original file.<br>
><br>
> ================<br>
> Comment at: lib/MC/MCParser/AsmParser.cpp:2749<br>
> @@ -2747,2 +2748,3 @@<br>
>      Lex();<br>
>      if (parseAbsoluteExpression(Val))<br>
> +      return false;<br>
> ----------------<br>
> Please decompose this into parseExpression and evaluateAsAbsolute to distinguish expression structure errors (should throw an error here) from value errors which we may defer.<br>
><br>
><br>
> Repository:<br>
>   rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D20337">http://reviews.llvm.org/D20337</a><br>
><br>
><br>
><br>
</p>