[cfe-commits] [Patch] PR11179 - Update StmtPrinter to not complain on some IntegerLiteral types

[Richard Trieu]

On Wed, Nov 2, 2011 at 12:36 AM, Enea Zaffanella <zaffanella at cs.unipr.it>wrote:

> Il 02/11/2011 03:21, Richard Trieu ha scritto:
> > On Mon, Oct 24, 2011 at 7:30 AM, Douglas Gregor <dgregor at apple.com
> > <mailto:dgregor at apple.com>> wrote:
> >
> >
> >     On Oct 20, 2011, at 5:36 PM, Chandler Carruth wrote:
> >
> >>     On Thu, Oct 20, 2011 at 5:12 PM, Richard Trieu <rtrieu at google.com
> >>     <mailto:rtrieu at google.com>> wrote:
> >>
> >>         Should Clang be printing suffixes that are accepted only with
> >>         certain flags?
> >>
> >>
> >>     I think this is an interesting policy decision. I'd love to hear
> >>     Doug's thoughts on it.
> >>
> >>     It seems fine to me for Clang, when running with -fms-extensions,
> >>     to suggest fixes even if only valid for -fms-extensions. Clearly
> >>     if there is a generic suggestion that could be made, that would be
> >>     a preferred alternative. For example, '__asm__' should be
> >>     suggested before 'asm'.
> >
> >     I think it's fine for Clang to print suffixes that are only accepted
> >     with certain flags. Presumably, you should never get an
> >     IntegerLiteral of type __int128_t unless you're in a dialect that
> >     supports parsing it.
> >
> >     … except that we cheat when we're building template arguments,
> >     because it was convenient. That cheating could be eliminated by
> >     encoding integer literal values directly within
> >     SubstNonTypeTemplateParmExpr.
> >
> >     - Doug
> >
> >
> > New patch.  Changes as follows:
> > Add int128 and uint128 suffixes (i128 and Ui128) to StmtPrinter.  short
> > and unsigned short will get to llvm_unreachable
> > Add assert to IntergerLiteral to prevent creation with type short or
> > unsigned short
> > Fix comment in IntegerLiteral to say that int128 and uint128 are
> > acceptable types
> > Change BuildExpressFromIntegralTemplateArgument to give a proper Expr.
> >  For negative numbers, use UnaryOperator of IntegerLiteral.  For short
> > and unsigned short, ImplicitCastExpr from int.
>
>
> The case of explictly signed/unsigned char template arguments should be
> considered too. Currently clang produces a character literal having a
> signed/unsigned char type in those cases, but such a character literal
> should typically have plain char type. Would it be possible to improve
> the current patch so that an integer literal is used instead (with the
> appropriate cast)?
>
> Yes, that is certainly possible to do. I will play around with it a little
and see how it works out.

>
> Regarding the kind of cast: I am fully aware that here it is mainly a
> matter of taste ... but wouldn't it be slightly better to use explicit
> casts, so that the required conversions are more clearly readable from
> the pretty printed statement?
>
> For instance, consider the following:
>
>  template <short v> struct S {
>    static const long a = v;
>  };
>
>  S<0> s;
>
> Here, when initializing the static member `a', the short value `v' is
> implicitly converted to long. If we go for implicit casts, we will
> obtain a template instance where the 0 integer literal is first
> implicitly converted to short and then implicitly converted to long,
> which is somehow strange looking. An explicit cast from the integer
> literal to short may serve as a better hint for the reader regarding the
> fact that such a 0 constant value was originally having type short.
>

This is what the AST dump looks like:

template <short v = 0> struct S {
    struct S;
    static const long a = (ImplicitCastExpr 0x4664d48
<unsignedtemplate.cc:24:26> 'const long' <IntegralCast>
  (SubstNonTypeTemplateParmExpr 0x4664d20 <col:26> 'short'
    (ImplicitCastExpr 0x4664d08 <col:26> 'short' <IntegralCast>
      (IntegerLiteral 0x4664ce0 <col:26> 'int' 0))))
;
    inline S() throw() (CompoundStmt 0x4664ff8 <unsignedtemplate.cc:23:28>)


    inline S(const S<0> &) throw();
}

The two implicit casts are separated by a SubstNonTypeTemplateParamExpr.
 It shows the integer literal cast to short to match the non-type template
parameter before being case to long for assignment.

>
> Cheers,
> Enea.
>
>
> > Basically, protect IntegerLiteral being shorts, fix the only case of
> > short IntegerLiterals, and add support for printing int128
> > IntegerLiterals.
> >
> > PR:
> > http://llvm.org/bugs/show_bug.cgi?id=11179
> >
> > Patch also located at:
> > http://codereview.appspot.com/5309045/
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111102/50977017/attachment.html>