This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [C PATCH] Fix -Wtautological-compare (PR c/82437; #2)


On Fri, Oct 06, 2017 at 02:21:41PM +0200, Jakub Jelinek wrote:
> On Thu, Oct 05, 2017 at 11:05:57PM +0200, Marek Polacek wrote:
> > On Thu, Oct 05, 2017 at 10:34:26PM +0200, Jakub Jelinek wrote:
> > > Hi!
> > > 
> > > In warn_tautological_bitwise_comparison, there is even a comment
> > > mentioning the fact that the types of the two constants might not be the
> > > same (it is called with the original arguments before they are promoted
> > > to common type for the comparison).
> > > 
> > > On the following testcase, one of the constants was unsigned (because
> > > it has been converted to that when anded with unsigned variable), while
> > > the other was signed and wi::to_widest (bitopcst) & wi::to_widest (cst)
> > > wasn't sign-extended from 32-bit (because bitopcst was unsigned),
> > > while wi::to_widest (cst) was (because cst was int), so we warned even
> > > when we shouldn't.
> > > 
> > > The following patch instead uses the precision of the larger type and
> > > zero extends from that (because we really don't need to care about bits
> > > beyond that precision).
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Looks ok, thanks.
> 
> Actually, after committing it I've realized it was wrong.  Sorry for that.
> 
> By forcing UNSIGNED sign in wide_int::from it forces zero extension from the
> precision of the tree to prec, but if there are any bits in between, we
> need extension according to the sign of the constant.  As we create the
> wide_int with prec, comparison of the wide_ints only looks at the low prec
> bits (or, if both wide_ints are guaranteed to be sign-extended from that
> the bits above will match too).
> So, the right thing to do is wide_int::from (cst, prec, TYPE_SIGN (TREE_TYPE (cst)))
> which is wi::to_wide (cst, prec).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

	Marek


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]