rSerge marked 5 inline comments as done.
rSerge 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)) {
----------------
dberris wrote:
> 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.
Changed.
https://reviews.llvm.org/D26415
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
