mgorny marked 2 inline comments as done. mgorny added a comment. In D47819#1356347 <https://reviews.llvm.org/D47819#1356347>, @Lekensteyn wrote:
> The direction of this patch looks reasonable to me. Is it worth mentioning > the issue (https://github.com/google/sanitizers/issues/974) in the commit > message? Yes, makes sense. I'll add it when committing. > What environment have you tested this in, could you verify that the tests are > indeed skipped on a system without these headers, and passed otherwise? I have only tests without those headers. I'll try to rebuild glibc with them enabled if it's still supported but it'd be preferable if somebody else tested it on 'natural' environment. ================ Comment at: lib/sanitizer_common/CMakeLists.txt:195 +set(SANITIZER_COMMON_DEFINITIONS + HAVE_RPC_XDR_H=${HAVE_RPC_XDR_H}) ---------------- Lekensteyn wrote: > The old code defined HAVE_xxx to 0 or 1, but in general wouldn't it be > preferable to define it to 1 if available, and not define it at all if > unavailable? This would match most other HAVE_xxx checks. > > In order to make the lit config work in this case, you have to use something > like `pythonize_bool(HAVE_RPC_XDR_H)`. Well, I suppose I could do that but given that the old code did it like this, the 0/1 definition seems to be a common practice in compiler-rt, and there is no apparent benefit from doing it the other way, I don't think it a good thing to do. ================ Comment at: test/lit.common.configured.in:39 set_default("android", @ANDROID_PYBOOL@) +set_default("have_rpc_xdr_h", @HAVE_RPC_XDR_H@) config.available_features.add('target-is-%s' % config.target_arch) ---------------- Lekensteyn wrote: > This becomes 0 or 1 instead of True or False, not sure if that will cause > issues later. It shouldn't. 0/1 evaluates the same in boolean context, and other LLVM tests are relying on 0/1 being passed to lit already. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D47819/new/ https://reviews.llvm.org/D47819 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
