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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
