lebedev.ri added a comment. @erichkeane, @rsmith thanks for taking a look!
================ Comment at: lib/CodeGen/CGExprScalar.cpp:1036 + return; + // That's it. We can't rule out any more cases with the data we have. + ---------------- rsmith wrote: > I don't like the overlap between the implicit truncation check and this > check. I think you should aim for exactly one of those checks to fire for any > given integer conversion. There are the following cases: > > * Dst is smaller than Src: if the value changes at all (with sign change or > without), then the truncation check already catches it, and catching it here > does not seem useful > * Dst is the same size as Src or larger: sign change is the only problem, > and is only possible if exactly one of Src and Dst is signed > > So I think you should bail out of this function if either Src and Dst are > both unsigned or both are signed, and also if Src is larger than Dst (because > we treat that case as a lossy truncation rather than as a sign change). > > And when you do emit a check here, the only thing you need to check is if the > signed value is negative (if so, you definitely changed the sign, and if not, > you definitely didn't -- except in the truncation cases that the truncation > sanitizer catches). To be clear: we want to skip emitting in those cases if the other check (truncation) is enabled, right? It does seem to make sense, (and i did thought about that a bit), but i need to think about it more.. ================ Comment at: lib/CodeGen/CGExprScalar.cpp:1050-1051 + // NOTE: if it is unsigned, then the comparison is naturally always 'false'. + llvm::ICmpInst::Predicate Pred = + VSigned ? llvm::ICmpInst::ICMP_SLT : llvm::ICmpInst::ICMP_ULT; + // Get the zero of the same type with which we will be comparing. ---------------- rsmith wrote: > If `!VSigned`, the result is a constant `false`; you don't need to emit an > `icmp` to work that out. Ok, if you insist. I didn't do that in the first place because we will now have an `icmp` where one operand being a constant, so we can simplify it further. And i don't want to complicate this logic if middle-end already handles it :) Repository: rC Clang https://reviews.llvm.org/D50250 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
