dberris added inline comments.
================ Comment at: lib/Driver/Tools.cpp:4903-4906 if (Triple.getOS() == llvm::Triple::Linux && (Triple.getArch() == llvm::Triple::arm || - Triple.getArch() == llvm::Triple::x86_64)) { + Triple.getArch() == llvm::Triple::x86_64 || + Triple.getArch() == llvm::Triple::aarch64)) { ---------------- rSerge wrote: > dberris wrote: > > rSerge wrote: > > > dberris wrote: > > > > I'm wondering whether it's worth turning this into a `switch` statement > > > > now that we have more than two supported architectures? > > > I think that would lead to more awkward code: there wouldn't be a single > > > decision outcome point (like the current `else` block), so to avoid > > > duplicating the code which currently prints the message, a `bool` > > > variable would be needed. I think it's more neat to just enumerate all > > > the OS&CPU combinations in the `if` condition for now. > > > This place is not performance-critical and the compiler should convert > > > appropriate `if`s into `switch`es anyway. > > This is an issue of making it more readable, something like: > > > > ``` > > if (Triple.getOS() != llvm::Triple::Linux) > > D.Diag(...) << ...; // Unsupported OS. > > switch (Triple.getArch()) { > > case llvm::Triple::arm: > > case llvm::Triple::x86_64: > > case llvm::Tripe::aarch64: > > // Supported. > > break; > > default: > > D.Diag(...) << ...; > > } > > ``` > > > > This way any new architectures become supported, they just get added to the > > list of cases that short-circuit. > We can't say that an OS is supported or unsupported unless all CPU > architectures for this OS support or don't support XRay, and this is not > going to happen in the near future. So it is more accurate to say about the > triple: some triples are supported and some are not. So in coding it is > natural to check for the triple with `||` and `&&`. > We can't say that an OS is supported or unsupported unless all CPU > architectures for this OS support or don't support XRay, and this is not > going to happen in the near future. I don't get it. Right now, as written, it doesn't matter what OS it is -- any OS other than Linux wouldn't be supported anyway. Maybe I mis-wrote, but: ``` if (Triple.getOS() != llvm::Triple::Linux) D.Diag(...) << ...; else switch(Triple.getArch()) { ... default: D.Diag(...) << ...; } ``` Is a direct translation that's more readable than the current complex if statement conditional. > So it is more accurate to say about the triple: some triples are supported > and some are not. So in coding it is natural to check for the triple with || > and &&. Sure, but conditional is already unwieldy with just three supported platforms. https://reviews.llvm.org/D26415 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits