r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.

[Bob Wilson]

> On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <ahatanak at gmail.com> 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?

> 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.

> 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.