<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 21, 2014, at 11:06 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 15, 2014 at 2:52 PM, Akira Hatanaka <span dir="ltr" class=""><<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="">Updated patch is attached. I made the following changes since I sent the last patch:<div class=""><br class=""></div><div class="">
- Added comments in GCCAsmStmt::AnalyzeAsmString to make it clearer what the string and the range represent.</div>
<div class="">- Fixed typo "modifer" in note_asm_modifier's string.<br class=""></div><div class=""><div class="">- Added test case in which '%' comes from a macro. If I use macros, clang doesn't seem to highlight the fixit range or print the string, but that is true for other kinds of warnings too.</div>
<div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">#define PERCENT "%"</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> ^ (no fixit string or range here)</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">3 warnings generated.</div></div><div class=""><br class=""></div></div><div class=""><div class="">RIchard, is this the "interesting" test case you were expecting?</div></div></div></blockquote>
<div class=""><br class=""></div><div class="">Yes, thank you!</div><div class=""><br class=""></div><div class="">Please add a CHECK-NOT: fix-it after your CHECK:s for this case. Other than that, this looks good to me, but someone else will need to confirm that the fix to add the 'w' modifier is correct; Eric?</div></div></div></div></div></blockquote><div><br class=""></div>Yes, the “w” modifier change is correct.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote">
<div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 14, 2014 at 2:09 PM, Richard Smith <span dir="ltr" class=""><<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <span dir="ltr" class=""><<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="">Updated patch attached.<br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote"><div class="">On Thu, Aug 14, 2014 at 11:51 AM, Richard Smith <span dir="ltr" class=""><<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>></span> wrote:<br class="">
<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" class=""><div class="">Thanks!</div><div class=""><br class=""></div><div class="">+ SourceRange getRange() const {<br class="">
</div><div class=""><br class=""></div><div class="">This should be a CharSourceRange; SourceRange is a token range. You're getting away with this because running the lexer within the string literal happens to skip to the end of the modifier, but there are almost certainly ways to observe this problem.</div>
<div class=""><br class=""></div><div class="">+ def note_asm_missing_constraint_modifier : Note<</div><div class="">+ "Use constraint modifer \"%0\"">;</div><div class=""><br class=""></div><div class="">Diagnostics should start with a lowercase letter.</div>
<div class=""><br class=""></div><div class="">Please add a testcase for weirder ways of writing the string literal; things like</div><div class=""><br class=""></div><div class=""> uint8_t byte, *addr;</div><div class=""> #define constraint(x) "%" #x</div><div class=""> #define mem(x) constraint([x])</div>
<div class=""> #define ldrb(x, y) "ldrb " x ", " y</div><div class=""> __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</div></div><div class=""><div class="">
<div class="gmail_extra">
<br class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div></div><div class="">I didn't understand what this piece of code is testing. Could you elaborate on it? When I compile it with '-fsyntax-only', clang complains that there is a missing closing parenthesis.</div>
<div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px;" class=""><font face="courier new, monospace" class=""><b class="">testmacro1.c:6:32: </b><span style="color:rgb(195,55,32)" class=""><b class="">error: </b></span><b class="">expected ')'</b></font></div><div class=""><div style="margin: 0px; font-size: 11px;" class=""><font face="courier new, monospace" class="">__asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</font></div>
</div><div style="margin: 0px; font-size: 11px; color: rgb(52, 189, 38);" class=""><b class=""><font face="courier new, monospace" class=""> ^</font></b></div><div style="margin: 0px; font-size: 11px;" class=""><font face="courier new, monospace" class=""><b class="">testmacro1.c:6:8: note: </b>to match this '('</font></div><div class=""><div style="margin: 0px; font-size: 11px;" class=""><font face="courier new, monospace" class="">__asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</font></div>
</div><div style="margin: 0px; font-size: 11px; color: rgb(52, 189, 38);" class=""><b class=""><font face="courier new, monospace" class=""> ^</font></b></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class="">1 error generated.</div><p style="margin:0px;font-size:11px;font-family:Menlo" class=""><span style="color:rgb(80,0,80);font-family:arial;font-size:small" class=""></span></p></div></div>
</div></div></blockquote><div class=""><br class=""></div></div><div class="">I would like a test where the source locations of different parts of the string literal are "interesting"; for instance, where the '%' and the following characters come from different parts of the source code.</div>
<div class=""><div class="">
<div class=""><span style="color:rgb(80,0,80)" class=""> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">
<div class=""><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=""><div class=""><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 14, 2014 at 8:54 AM, Akira Hatanaka <span dir="ltr" class=""><<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>></span> wrote:<br class="">
<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" class="">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 class=""><div class=""><div class=""><br class=""></div><div class="gmail_extra">
<div class="gmail_quote">
On Wed, Aug 13, 2014 at 11:38 AM, Richard Smith <span dir="ltr" class=""><<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>></span> wrote:<br class="">
<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" class=""><div class="">This looks completely wrong:</div><div class="">
<br class=""></div>+ LocStart.getLocWithOffset(DiagOffs)));<br class="">
<div class="gmail_extra"><br class=""></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 class=""></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 class=""><br class=""></div><div class="">Likewise here.</div><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class=""><div class="">+ __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");</div><div class="">+// CHECK: warning: value size does not match register size specified by the constraint and modifier</div>
<div class="">+// CHECK:{{^}} ^</div><div class="">+// CHECK:{{^}} "%w0"</div></div><div class=""><br class=""></div><div class="">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 class=""><div class="gmail_quote"><div class=""><div class="">On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <span dir="ltr" class=""><<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>></span> wrote:<br class="">
</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 class=""><div class="">
<div dir="ltr" class="">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 class=""><div class=""><div class="gmail_extra">
<br class=""><br class=""><div class="gmail_quote">
On Tue, Aug 12, 2014 at 4:51 PM, Akira Hatanaka <span dir="ltr" class=""><<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>></span> wrote:<br class=""><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" class=""><div class=""><div class="">On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <span dir="ltr" class=""><<a href="mailto:bob.wilson@apple.com" target="_blank" class="">bob.wilson@apple.com</a>></span> wrote:<br class=""></div></div><div class="gmail_extra">
<div class="gmail_quote"><div class=""><div class="">
<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 class="">
> On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank" class="">ahatanak@gmail.com</a>> wrote:<br class="">
><br class="">
> The attached patch improves upon the patch James sent out a few weeks ago.<br class="">
><br class="">
> 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 class="">
><br class="">
<br class="">
</div>Some comments/questions….<br class="">
<br class="">
> diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h<br class="">
> index 790c8e3..9ce6d22 100644<br class="">
> --- a/include/clang/AST/Stmt.h<br class="">
> +++ b/include/clang/AST/Stmt.h<br class="">
> @@ -1580,18 +1580,17 @@ public:<br class="">
> Kind MyKind;<br class="">
> std::string Str;<br class="">
> unsigned OperandNo;<br class="">
> + SourceLocation Loc;<br class="">
> public:<br class="">
> AsmStringPiece(const std::string &S) : MyKind(String), Str(S) {}<br class="">
> - AsmStringPiece(unsigned OpNo, char Modifier)<br class="">
> - : MyKind(Operand), Str(), OperandNo(OpNo) {<br class="">
> - Str += Modifier;<br class="">
> + AsmStringPiece(unsigned OpNo, const std::string &S, SourceLocation L)<br class="">
> + : MyKind(Operand), Str(S), OperandNo(OpNo), Loc(L) {<br class="">
> }<br class="">
><br class="">
> bool isString() const { return MyKind == String; }<br class="">
> bool isOperand() const { return MyKind == Operand; }<br class="">
><br class="">
> const std::string &getString() const {<br class="">
> - assert(isString());<br class="">
> return Str;<br class="">
> }<br class="">
><br class="">
<br class="">
Why did you remove the isString() assert?<br class="">
<br class=""></blockquote><div class=""><br class=""></div></div></div><div class="">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 class="">
<div class=""> <br class=""></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 class="">
> h<br class="">
> index a7b2ba2..4d53ade 100644<br class="">
> --- a/include/clang/Basic/Diagnostic.h<br class="">
> +++ b/include/clang/Basic/Diagnostic.h<br class="">
> @@ -773,6 +773,10 @@ private:<br class="">
> /// or modify at a particular position.<br class="">
> SmallVector<FixItHint, 8> DiagFixItHints;<br class="">
><br class="">
> + /// \brief Caret location. This is set only when the caret location is<br class="">
> + /// different from the current diagnostic location in CurDiagLoc.<br class="">
> + SourceLocation CaretLoc;<br class="">
> +<br class="">
> DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {<br class="">
> bool isPragma = L.isValid();<br class="">
> DiagnosticMapping Mapping =<br class="">
><br class="">
<br class="">
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 class="">
<br class=""></blockquote><div class=""><br class=""></div></div><div class="">I didn't think of using the existing location, but that sounds like a better solution.</div><div class=""><div class=""><div class=""> <br class=""></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 class="">
> nd/DiagnosticRenderer.h<br class="">
> index ce1dc90..1a4cae4 100644<br class="">
> --- a/include/clang/Frontend/DiagnosticRenderer.h<br class="">
> +++ b/include/clang/Frontend/DiagnosticRenderer.h<br class="">
> @@ -88,7 +88,8 @@ protected:<br class="">
> DiagnosticsEngine::Level Level,<br class="">
> SmallVectorImpl<CharSourceRange>& Ranges,<br class="">
> ArrayRef<FixItHint> Hints,<br class="">
> - const SourceManager &SM) = 0;<br class="">
> + const SourceManager &SM,<br class="">
> + SourceLocation CaretLoc) = 0;<br class="">
><br class="">
> virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,<br class="">
> const SourceManager &SM) = 0;<br class="">
> @@ -116,7 +117,7 @@ private:<br class="">
> void emitModuleBuildStack(const SourceManager &SM);<br class="">
> void emitCaret(SourceLocation Loc, DiagnosticsEngine::Level Level,<br class="">
> ArrayRef<CharSourceRange> Ranges, ArrayRef<FixItHint> Hints,<br class="">
> - const SourceManager &SM);<br class="">
> + const SourceManager &SM, SourceLocation CaretLoc);<br class="">
> void emitMacroExpansions(SourceLocation Loc,<br class="">
> DiagnosticsEngine::Level Level,<br class="">
> ArrayRef<CharSourceRange> Ranges,<br class="">
> @@ -132,17 +133,19 @@ public:<br class="">
> /// information needed based on macros whose expansions impact the<br class="">
> /// diagnostic.<br class="">
> ///<br class="">
> - /// \param Loc The location for this caret.<br class="">
> + /// \param Error location.<br class="">
><br class="">
<br class="">
You dropped the parameter name here.</blockquote></div></div></div><br class=""></div><div class="gmail_extra">I'll fix this and send a new patch.</div><div class="gmail_extra"><br class=""></div></div>
</blockquote></div><br class=""></div>
</div></div><br class=""></div></div><div class="">_______________________________________________<br class="">
cfe-commits mailing list<br class="">
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" class="">cfe-commits@cs.uiuc.edu</a><br class="">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br class="">
<br class=""></div></blockquote></div><br class=""></div></div>
</blockquote></div><br class=""></div></div></div></div>
</blockquote></div><br class=""></div>
</div></div></blockquote></div></div></div><br class=""></div></div>
</blockquote></div></div></div><br class=""></div></div>
</blockquote></div><br class=""></div>
</div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>