[cfe-commits] [llvm-commits] [PATCH] Rename llvm::Triple::ANDROIDEABI to llvm::Triple::Android

[Logan Chien]

Hi,

    I have read the code again.  It seems that the additional guard
for *Env == "android"* is not necessary as well, since isEABI() is a member
function of ARMABIInfo, which will only be created when the architecture
is ARM or Thumb.  Thus, "i686-none-linux-android" will not be considered
as EABI as well.  I hope this answers your earliest question.  Thanks.

Regards,
-Logan


On Mon, Sep 3, 2012 at 10:55 PM, Evgeniy Stepanov <eugeni.stepanov at gmail.com
> wrote:

> On Mon, Sep 3, 2012 at 2:42 PM, Logan Chien <tzuhsiang.chien at gmail.com>
> wrote:
> > Hi,
> >
> >   Yes.  Currently, only arm-linux-androideabi is used.  However
> > I have 2 concerns:
> >
> > 1. I'm afraid that it will be confusing to the user that clang supports
> > arm-linux-androideabi, i686-linux-android, mipsel-linux-android, but
> > NOT arm-linux-android.
>
> I'm not sure what arm-linux-android means at all. There are no
> platforms with this triple. We can either invent new meaning for this
> combination (say, make it a synonym to arm-linux-androideabi, or to
> plain arm-linux), or make it explicitly unsupported.
>
> Maybe we should just take your earlier suggestion and patch isEABI
> such that arm-linux-android is a eabi, and the rest aren't.
>
> >
> > 2. From the LLVM/Clang developer point of view, adding one additional
> > case will become error-prone.  (see the switch cases in the attached
> patch)
> >
> > I have created the new patch set to split Android into Android and
> > AndroidEABI
> > as you have suggested.  However, I'm not sure it is a good idea or not,
> > please let me know if you have different thought or if I did anything
> wrong.
> > Your comments are appreciated.  Thanks.
> >
> > Sincerely,
> > Logan
> >
> > p.s. For your convenience,
> >
> https://github.com/loganchien/llvm-logan/compare/master...logan-androideabi
> >
> https://github.com/loganchien/clang-logan/compare/master...logan-androideabi
> >
> >
> > On Mon, Sep 3, 2012 at 5:48 PM, Evgeniy Stepanov <
> eugeni.stepanov at gmail.com>
> > wrote:
> >>
> >> Do we need to support arm-linux-android at all? AFAIK, it's always
> >> arm-linux-androideabi.
> >>
> >> On Mon, Sep 3, 2012 at 12:48 PM, Logan Chien <tzuhsiang.chien at gmail.com
> >
> >> wrote:
> >> > I did hesitate while adding "android" to isEABI as well.
> >> >
> >> > The problem I am facing is that I wish isEABI("arm-linux-android") ==
> >> > true,
> >> > isEABI("i686-unknown-linux-android") == false.  After some
> >> > consideration,
> >> > I thought it is strange to check isEABI() on non-ARM architecture,
> thus
> >> > I
> >> > chose to add "android".
> >> >
> >> > BTW, I'm afraid that adding a new different environment type won't
> help.
> >> > Because we want "-android" considered as EABI on ARM, but not on X86.
> >> >
> >> > If we should exclude non-ARM architecture, how about changing the code
> >> > into:
> >> >
> >> > bool isEABI() const {
> >> >   const llvm::Triple& TT = getContext().getTargetInfo().getTriple();
> >> >   StringRef Env = TT.getEnvironmentName();
> >> >
> >> >   return (Env == "gnueabi" || Env == "eabi" || Env == "androideabi" ||
> >> >           ((TT.getArch() == llvm::Triple::arm ||
> >> >             TT.getArch() == llvm::Triple::thumb)) && Env ==
> "android"));
> >> > }
> >> >
> >> > Or do you have better suggestion?  Thanks for your review.
> >> >
> >> > Sincerely,
> >> > Logan
> >> >
> >> >
> >> >
> >> > On Mon, Sep 3, 2012 at 4:16 PM, Evgeniy Stepanov
> >> > <eugeni.stepanov at gmail.com>
> >> > wrote:
> >> >>
> >> >> The following chunk looks weird to me.
> >> >>
> >> >>    bool isEABI() const {
> >> >>      StringRef Env =
> >> >>        getContext().getTargetInfo().getTriple().getEnvironmentName();
> >> >> -    return (Env == "gnueabi" || Env == "eabi" || Env ==
> >> >> "androideabi");
> >> >> +    return (Env == "gnueabi" || Env == "eabi" ||
> >> >> +            Env == "android" || Env == "androideabi");
> >> >>    }
> >> >>
> >> >> Now we have isEABI() == true for i686-linux-android.
> >> >> I'd rather we kept more information about the original triple by
> >> >> having both android and androideabi environments, and a helper
> >> >> function isAndroid() somewhere.
> >> >>
> >> >> On Sun, Sep 2, 2012 at 1:48 PM, Logan Chien <
> tzuhsiang.chien at gmail.com>
> >> >> wrote:
> >> >> > Thanks.  Commited as r163087, r163088.
> >> >> > -Logan
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Fri, Aug 31, 2012 at 5:25 PM, Anton Korobeynikov
> >> >> > <anton at korobeynikov.info> wrote:
> >> >> >>
> >> >> >> > This is because that we are using "StartsWith" to translate
> >> >> >> > environment
> >> >> >> > name
> >> >> >> > (eg. andriodeabi, android, gnueabi, ...etc) into
> >> >> >> > Triple::EnvironmentType.
> >> >> >> > Thus,
> >> >> >> > I believe it won't case backward compatibility issue.  :-)
> >> >> >> Ok. LGTM then :)
> >> >> >>
> >> >> >> --
> >> >> >> With best regards, Anton Korobeynikov
> >> >> >> Faculty of Mathematics and Mechanics, Saint Petersburg State
> >> >> >> University
> >> >> >
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > 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/20120904/148b77c6/attachment.html>