<div dir="ltr">Updated patch which addresses comments from Eric and Richard is attached. Also, you can see the diagnostic messages clang currently prints in diag1.txt.<div><br></div><div class="gmail_extra"><div class="gmail_quote">
On Wed, Aug 13, 2014 at 11:38 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>This looks completely wrong:</div><div><br></div>+ LocStart.getLocWithOffset(DiagOffs)));<br>
<div class="gmail_extra"><br></div><div class="gmail_extra">As far as I can see, DiagOffs is an offset within the string, not an offset within the source file. This will do the wrong thing if the string literal is even slightly interesting (if there are character escapes, escaped newlines, trigraphs, string literal concatenation, macros, ...). Instead you should use StringLiteral::getLocationOfByte to find the SourceLocation of a particular character in the string literal.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra"><div class="gmail_extra">+ B.AddFixItHint(FixItHint::CreateInsertion(Loc.getLocWithOffset(-1),</div><div class="gmail_extra">+ SuggestedModifier));</div>
<div><br></div><div>Likewise here.</div><div><br></div><div>Also, this is adding a fix-it hint to a warning. That's only permitted if applying the fix-it hint would not change the semantics of the code (because fix-its on errors get automatically applied in some cases, and in order to support this, it's important that we recover as if all fix-its on errors were applied, and warnings are sometimes upgraded to errors). Otherwise, the fix-it should be on a separate note.</div>
<div><br></div><div><div>+ __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");</div><div>+// CHECK: warning: value size does not match register size specified by the constraint and modifier</div>
<div>+// CHECK:{{^}} ^</div><div>+// CHECK:{{^}} "%w0"</div></div><div><br></div><div>Please check your fixit with -fdiagnostics-parseable-fixits rather than by FileChecking the user-facing output. See the existing tests in test/FixIt for examples.</div>
</div><br><div class="gmail_quote"><div><div>On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
</div></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"><div><div>
<div dir="ltr">Updated patch is attached. As per Bob's suggestion, I changed it to use the existing diagnostic location instead of defining a separate location for the caret.</div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">
On Tue, Aug 12, 2014 at 4:51 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><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 dir="ltr"><div><div>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></div><div class="gmail_extra">
<div class="gmail_quote"><div><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"><div><br>
> On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">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></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>
<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><div>I didn't think of using the existing location, but that sounds like a better solution.</div><div><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></div></div><br></div><div class="gmail_extra">I'll fix this and send a new patch.</div><div class="gmail_extra"><br></div></div>
</blockquote></div><br></div>
</div></div><br></div></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div>