Re: [patch]: Add support of new __int128 type for targets having 128-bit integer scalar support

[Kai Tietz <ktietz70 at googlemail dot com>]

2010/4/16 Joseph S. Myers <joseph@codesourcery.com>:
> On Fri, 16 Apr 2010, Kai Tietz wrote:
>
>> 2010/4/15 Joseph S. Myers <joseph@codesourcery.com>:
>> > You don't appear to have addressed my previous review comments in this
>> > patch version (for example, the point that __int128 should be handled like
>> > int, void, _Bool etc., getting "two or more data types in declaration
>> > specifiers" for use with those specifiers instead of having explicit
>> > checks for all of them). ?Furthermore, there are changes to existing code
>> > (patch hunks that just change whitespace, for example) that appear
>> > irrelevant to the addition of __int128.
>> >
>> > --
>> > Joseph S. Myers
>> > joseph@codesourcery.com
>> >
>>
>> Hmm, I just re-read my patch I sent and what white-space changes you
>> are talking about? I don't see in this patch pure white-space changes.
>
> See the @@ -8725,6 +8738,7 @@ hunk for example, inserting a blank line, or
> @@ -9024,7 +9105,7 @@, changing indentation. ?I refer to
> <http://gcc.gnu.org/ml/gcc-patches/2010-04/msg00354/int128doc.diff>.

I see, but I don't see an new-line in this hunk. I see that leading
spaces got converted to tabs. And I admit this is an unrelated hunk
for this patch.
At another place I added an new line to for read-ability, but I revert
it in my updated patch.

>> Also I modified in this patch that __int128 is handled like void,
>> _Bool, int, float, etc. I don't see you point. Could it be that you've
>
> If it's handled like them, you wouldn't need special diagnostics such as
> "both %<__int128%> and %<char%> in declaration specifiers" because the
> generic diagnostic I mentioned would have caught this earlier. ?And the
> check in the RID_LONG case would go with all the other checks for
> particular keywords such as _Bool, rather than before the "long long long"
> check in your present patch; likewise for RID_SHORT and probably other
> cases.
I see and indeed it is useless code as condition will never hit as it
is checked already before.

>?The falling through from cts_int128 to cts_int is also suspicious
> since almost none of the code is actually relevant to both cases.

Yeah, this fall-through to int-case wasn't wrong in terms of logic but
piggy as the check for int128 in this case is misleading. The check in
short-case is superfluous.

New patch tested for i686-pc-linux-gnu, i686-pc-cygwin,
i686-pc-mingw32, x86_64-pc-mingw32, and x86_64-pc-linux-gnu. Ok for
apply?

Thanks,
Kai


-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Attachment: int128doc.diff
Description: Binary data