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