> On Aug 21, 2014, at 11:06 AM, Richard Smith <[email protected]> wrote: > > On Fri, Aug 15, 2014 at 2:52 PM, Akira Hatanaka <[email protected] > <mailto:[email protected]>> wrote: > Updated patch is attached. I made the following changes since I sent the last > patch: > > - Added comments in GCCAsmStmt::AnalyzeAsmString to make it clearer what the > string and the range represent. > - Fixed typo "modifer" in note_asm_modifier's string. > - 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. > > #define PERCENT "%" > ^ (no fixit string or range here) > 3 warnings generated. > > RIchard, is this the "interesting" test case you were expecting? > > Yes, thank you! > > 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?
Yes, the “w” modifier change is correct. > > On Thu, Aug 14, 2014 at 2:09 PM, Richard Smith <[email protected] > <mailto:[email protected]>> wrote: > On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <[email protected] > <mailto:[email protected]>> wrote: > Updated patch attached. > > On Thu, Aug 14, 2014 at 11:51 AM, Richard Smith <[email protected] > <mailto:[email protected]>> wrote: > Thanks! > > + SourceRange getRange() const { > > 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. > > + def note_asm_missing_constraint_modifier : Note< > + "Use constraint modifer \"%0\"">; > > Diagnostics should start with a lowercase letter. > > Please add a testcase for weirder ways of writing the string literal; things > like > > uint8_t byte, *addr; > #define constraint(x) "%" #x > #define mem(x) constraint([x]) > #define ldrb(x, y) "ldrb " x ", " y > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > > > > 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. > > testmacro1.c:6:32: error: expected ')' > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > ^ > testmacro1.c:6:8: note: to match this '(' > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > ^ > 1 error generated. > > 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. > > On Thu, Aug 14, 2014 at 8:54 AM, Akira Hatanaka <[email protected] > <mailto:[email protected]>> wrote: > Updated patch which addresses comments from Eric and Richard is attached. > Also, you can see the diagnostic messages clang currently prints in diag1.txt. > > On Wed, Aug 13, 2014 at 11:38 AM, Richard Smith <[email protected] > <mailto:[email protected]>> wrote: > This looks completely wrong: > > + LocStart.getLocWithOffset(DiagOffs))); > > 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. > > + B.AddFixItHint(FixItHint::CreateInsertion(Loc.getLocWithOffset(-1), > + SuggestedModifier)); > > Likewise here. > > 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. > > + __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory"); > +// CHECK: warning: value size does not match register size specified by the > constraint and modifier > +// CHECK:{{^}} ^ > +// CHECK:{{^}} "%w0" > > 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. > > On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <[email protected] > <mailto:[email protected]>> wrote: > 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. > > > On Tue, Aug 12, 2014 at 4:51 PM, Akira Hatanaka <[email protected] > <mailto:[email protected]>> wrote: > On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <[email protected] > <mailto:[email protected]>> wrote: > > > On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <[email protected] > > <mailto:[email protected]>> wrote: > > > > The attached patch improves upon the patch James sent out a few weeks ago. > > > > 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. > > > > Some comments/questions…. > > > diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h > > index 790c8e3..9ce6d22 100644 > > --- a/include/clang/AST/Stmt.h > > +++ b/include/clang/AST/Stmt.h > > @@ -1580,18 +1580,17 @@ public: > > Kind MyKind; > > std::string Str; > > unsigned OperandNo; > > + SourceLocation Loc; > > public: > > AsmStringPiece(const std::string &S) : MyKind(String), Str(S) {} > > - AsmStringPiece(unsigned OpNo, char Modifier) > > - : MyKind(Operand), Str(), OperandNo(OpNo) { > > - Str += Modifier; > > + AsmStringPiece(unsigned OpNo, const std::string &S, SourceLocation L) > > + : MyKind(Operand), Str(S), OperandNo(OpNo), Loc(L) { > > } > > > > bool isString() const { return MyKind == String; } > > bool isOperand() const { return MyKind == Operand; } > > > > const std::string &getString() const { > > - assert(isString()); > > return Str; > > } > > > > Why did you remove the isString() assert? > > > 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". > > > diff --git a/include/clang/Basic/Diagnostic.h > > b/include/clang/Basic/Diagnostic.\ > > h > > index a7b2ba2..4d53ade 100644 > > --- a/include/clang/Basic/Diagnostic.h > > +++ b/include/clang/Basic/Diagnostic.h > > @@ -773,6 +773,10 @@ private: > > /// or modify at a particular position. > > SmallVector<FixItHint, 8> DiagFixItHints; > > > > + /// \brief Caret location. This is set only when the caret location is > > + /// different from the current diagnostic location in CurDiagLoc. > > + SourceLocation CaretLoc; > > + > > DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) { > > bool isPragma = L.isValid(); > > DiagnosticMapping Mapping = > > > > 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. > > > I didn't think of using the existing location, but that sounds like a better > solution. > > > diff --git a/include/clang/Frontend/DiagnosticRenderer.h > > b/include/clang/Fronte\ > > nd/DiagnosticRenderer.h > > index ce1dc90..1a4cae4 100644 > > --- a/include/clang/Frontend/DiagnosticRenderer.h > > +++ b/include/clang/Frontend/DiagnosticRenderer.h > > @@ -88,7 +88,8 @@ protected: > > DiagnosticsEngine::Level Level, > > SmallVectorImpl<CharSourceRange>& Ranges, > > ArrayRef<FixItHint> Hints, > > - const SourceManager &SM) = 0; > > + const SourceManager &SM, > > + SourceLocation CaretLoc) = 0; > > > > virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc, > > const SourceManager &SM) = 0; > > @@ -116,7 +117,7 @@ private: > > void emitModuleBuildStack(const SourceManager &SM); > > void emitCaret(SourceLocation Loc, DiagnosticsEngine::Level Level, > > ArrayRef<CharSourceRange> Ranges, ArrayRef<FixItHint> > > Hints, > > - const SourceManager &SM); > > + const SourceManager &SM, SourceLocation CaretLoc); > > void emitMacroExpansions(SourceLocation Loc, > > DiagnosticsEngine::Level Level, > > ArrayRef<CharSourceRange> Ranges, > > @@ -132,17 +133,19 @@ public: > > /// information needed based on macros whose expansions impact the > > /// diagnostic. > > /// > > - /// \param Loc The location for this caret. > > + /// \param Error location. > > > > You dropped the parameter name here. > > I'll fix this and send a new patch. > > > > _______________________________________________ > cfe-commits mailing list > [email protected] <mailto:[email protected]> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits> > > > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
