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

[Logan Chien]

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.

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/20120903/3bcb0ccb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CFE_0001-Split-Android-to-two-cases.patch
Type: application/octet-stream
Size: 6536 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120903/3bcb0ccb/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: LLVM_0001-Split-Android-to-two-cases.patch
Type: application/octet-stream
Size: 2753 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120903/3bcb0ccb/attachment-0001.obj>