Re: [PATCH v2] Expand bitpos and type.length to LONGEST and ULONGEST

Siddhesh Poyarekar
2012-05-16T03:50:11Z

On Tue, 15 May 2012 22:04:54 +0200, Jan wrote:
> while I have a local patch for it it is easier to state that:
> * the patch needs a small rebase to HEAD for very recent changes.
> * it does not compile on 32-bit i386 host OS due to %ld used for
> LONGEST.
>

Thanks for the very exhaustive review. I'll need some time to go
through all of the points below, but I'll come up with a new patch once
I have.

> In such case I think we can keep signedness for all the changes.
> While one may consider more clean to use unsigned types in some cases
> it can bring unexpected regressions, for example in
> amd64_push_arguments you correctly changed the first occurence:
>
> > - int len = TYPE_LENGTH (type);
> > + LONGEST len = TYPE_LENGTH (type);
>
> But in other cases you use ULONGEST. In this case ULONGEST would be
> a regression as there is below:
>
> for (j = 0; len > 0; j++, len -= 8)
>
> If you have carefully checked all the signedness changes for possible
> regressions I will re-review it. Still it seems to me unrelated to
> this patch (and possibly not worth the effort, YMMV).
> Or did you see some specific splint warnings for it?
> I have marked in this patch such lines by ^X$.
> I understand changing to patch to keep the signedness looks like
> worsening the patch.

I think I agree with you on this because I had not thought of the
difficulty involved in isolating regressions due to the size of this
patch. It would be safer in this case to keep length as LONGEST because
while I did try to check for all cases where ULONGEST may cause a
regression (like above), but I cannot say for sure that it's all
perfect. The splint warnings are actually for the opposite (for length
not being ULONGEST), since splint is not that smart in these aspects.

Funnily, I think it will be easier because splint will be happy about
the fact that both bitpos and length are the same type. I'll work on
that and the rest of the review comments and come up with the next draft
for this patch.

Thanks,
Siddhesh