On Mon, Sep 3, 2012 at 2:42 PM, Logan Chien <[email protected]> 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 <[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-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
