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]> wrote: > On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <[email protected]> wrote: > >> >> > On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <[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. > >
inline-asm-modifier2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
