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

[Eric Christopher]

On Wed, Aug 13, 2014 at 10:08 AM, Eric Christopher <echristo at gmail.com> wrote:
> (I see you took Richard off of the review thread after I added him
> explicitly. Please don't do that.)
>
> Couple of comments from looking:
>
> +  assert(isOperand());
>
> Needs some text here as to why you're asserting.
>
>      // Handle %x4 and %x[foo] by capturing x as the modifier character.
> -    char Modifier = '\0';
> +    const char *Begin = CurPtr - 1;
>
> The uses of Begin could use some commenting, it's not obvious what's
> going on. Also you don't appear to capture modifier any more making
> the comment invalid.
>
> +                             std::string &SuggestedModifier) const override {
>
> StringRef?
>

Enh, it's an out parameter, nevermind on this part.

-eric

> +    const TargetInfo &TI = Context.getTargetInfo();
> +    if (!TI.validateConstraintModifier(Literal->getString(),
>
> You don't use TI anywhere else so you can fold this in.
>
> -eric
>
> On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <ahatanak at gmail.com> 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 <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.
>>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>