<div dir="ltr">On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</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"><div class=""><br>
> On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> wrote:<br>
><br>
> The attached patch improves upon the patch James sent out a few weeks ago.<br>
><br>
> It changes validateConstraintModifier to return the suggested modifier and also makes changes to print the caret and the suggested modifier right below the operand in the assembly template.<br>
><br>
<br>
</div>Some comments/questions….<br>
<br>
> diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h<br>
> index 790c8e3..9ce6d22 100644<br>
> --- a/include/clang/AST/Stmt.h<br>
> +++ b/include/clang/AST/Stmt.h<br>
> @@ -1580,18 +1580,17 @@ public:<br>
>      Kind MyKind;<br>
>      std::string Str;<br>
>      unsigned OperandNo;<br>
> +    SourceLocation Loc;<br>
>    public:<br>
>      AsmStringPiece(const std::string &S) : MyKind(String), Str(S) {}<br>
> -    AsmStringPiece(unsigned OpNo, char Modifier)<br>
> -      : MyKind(Operand), Str(), OperandNo(OpNo) {<br>
> -      Str += Modifier;<br>
> +    AsmStringPiece(unsigned OpNo, const std::string &S, SourceLocation L)<br>
> +      : MyKind(Operand), Str(S), OperandNo(OpNo), Loc(L) {<br>
>      }<br>
><br>
>      bool isString() const { return MyKind == String; }<br>
>      bool isOperand() const { return MyKind == Operand; }<br>
><br>
>      const std::string &getString() const {<br>
> -      assert(isString());<br>
>        return Str;<br>
>      }<br>
><br>
<br>
Why did you remove the isString() assert?<br>
<br></blockquote><div><br></div><div>When GCCAsmStmt::AnalyzeAsmString parses the "%0" or "%[s0]", it constructs an AsmStringPiece that is an Operand (i.e., MyKind is "Operand", not "String"). Later when Sema::ActOnGCCAsmStmt calls getString to create the fixit string, it asserts because it isn't a "String".</div>
<div> <br></div><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">
> diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.\<br>
> h<br>
> index a7b2ba2..4d53ade 100644<br>
> --- a/include/clang/Basic/Diagnostic.h<br>
> +++ b/include/clang/Basic/Diagnostic.h<br>
> @@ -773,6 +773,10 @@ private:<br>
>    /// or modify at a particular position.<br>
>    SmallVector<FixItHint, 8> DiagFixItHints;<br>
><br>
> +  /// \brief Caret location. This is set only when the caret location is<br>
> +  /// different from the current diagnostic location in CurDiagLoc.<br>
> +  SourceLocation CaretLoc;<br>
> +<br>
>    DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {<br>
>      bool isPragma = L.isValid();<br>
>      DiagnosticMapping Mapping =<br>
><br>
<br>
Can you explain more why you need a separate SourceLocation for this? i.e., Why not just set the existing diagnostic location? It seems like a large part of this patch is related to handling this separate caret location, and I’d like to understand why that is needed.<br>

<br></blockquote><div><br></div><div>I didn't think of using the existing location, but that sounds like a better solution.</div><div> <br></div><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">

> diff --git a/include/clang/Frontend/DiagnosticRenderer.h b/include/clang/Fronte\<br>
> nd/DiagnosticRenderer.h<br>
> index ce1dc90..1a4cae4 100644<br>
> --- a/include/clang/Frontend/DiagnosticRenderer.h<br>
> +++ b/include/clang/Frontend/DiagnosticRenderer.h<br>
> @@ -88,7 +88,8 @@ protected:<br>
>                                 DiagnosticsEngine::Level Level,<br>
>                                 SmallVectorImpl<CharSourceRange>& Ranges,<br>
>                                 ArrayRef<FixItHint> Hints,<br>
> -                               const SourceManager &SM) = 0;<br>
> +                               const SourceManager &SM,<br>
> +                               SourceLocation CaretLoc) = 0;<br>
><br>
>    virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,<br>
>                                     const SourceManager &SM) = 0;<br>
> @@ -116,7 +117,7 @@ private:<br>
>    void emitModuleBuildStack(const SourceManager &SM);<br>
>    void emitCaret(SourceLocation Loc, DiagnosticsEngine::Level Level,<br>
>                   ArrayRef<CharSourceRange> Ranges, ArrayRef<FixItHint> Hints,<br>
> -                 const SourceManager &SM);<br>
> +                 const SourceManager &SM, SourceLocation CaretLoc);<br>
>    void emitMacroExpansions(SourceLocation Loc,<br>
>                             DiagnosticsEngine::Level Level,<br>
>                             ArrayRef<CharSourceRange> Ranges,<br>
> @@ -132,17 +133,19 @@ public:<br>
>    /// information needed based on macros whose expansions impact the<br>
>    /// diagnostic.<br>
>    ///<br>
> -  /// \param Loc The location for this caret.<br>
> +  /// \param Error location.<br>
><br>
<br>
You dropped the parameter name here.</blockquote></div><br></div><div class="gmail_extra">I'll fix this and send a new patch.</div><div class="gmail_extra"><br></div></div>