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 <[email protected]>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 <[email protected]> > 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 < > [email protected]> > > 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 <[email protected]> > >> wrote: > >> > Thanks. Commited as r163087, r163088. > >> > -Logan > >> > > >> > > >> > > >> > On Fri, Aug 31, 2012 at 5:25 PM, Anton Korobeynikov > >> > <[email protected]> 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 > >> > [email protected] > >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> > > > > > >
CFE_0001-Split-Android-to-two-cases.patch
Description: Binary data
LLVM_0001-Split-Android-to-two-cases.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
