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

[Akira Hatanaka]

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 <ahatanak at gmail.com> wrote:

> On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>>
>> > 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?
>>
>>
> 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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140812/199f2dd6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inline-asm-modifier2.patch
Type: application/octet-stream
Size: 8385 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140812/199f2dd6/attachment.obj>