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

[Evgeniy Stepanov]

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
>> >> >
>> >
>> >
>
>