rSerge added a comment. In https://reviews.llvm.org/D24799#563058, @dberris wrote:
> Are we sure this will not fail on Windows? i.e. have you built/run the tests > on Windws ARM or X86_64? The test itself passes for `arm-pc-win32` and `x86_64-pc-win32` on my machine. So in this sense it doesn't fail. However, the feature of this patch doesn't cover the OS part of the triple: currently because of `mprotect` and other Linux-only code, XRay is only supported on Linux. However, clang (also with this patch) doesn't check for Linux target, and on non-Linux machines either LLVM backend will emit an error later (saying that XRay is not supported on Thumb - this happens because Windows is Thumb-only for ARM target), or I expect linker errors because code generation on Windows references some API of compiler-rt, which doesn't get compiled on Windows, even if it's x86_64. > dberris wrote in Tools.cpp:4787 > Why can't this just be a `const string`, or a `const StringRef`? Is there any advantage over `const char* const` here? > dberris wrote in Tools.cpp:4796 > I'm not a fan of calling `.data()` here -- if this returns a `StringRef` I'd > much rather turn it into a string directly. Either that or you can even > string these together. Say something like: > > default: > D.Diag(...) << (XRayInstrumentOption + " on " + > Triple.getArchName().str()); > break; It returns `StringRef`. `.str()` would construct a `std::string`, which seems unnecessary. Of course this piece is not performance-critical, but why not to just use `operator+=` taking as the argument `const char* const`? > dberris wrote in Tools.cpp:4799 > As part of my submitting this change upstream, I had to format it accordingly > with `clang-format` -- you might want to do the same so that the formatting > is in-line with the LLVM developers style guidelines. Done. https://reviews.llvm.org/D24799 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
