[cfe-dev] patch for generalizing stdint for n-bit bytes

[Chris Lattner]

On Oct 22, 2009, at 5:12 AM, Ken Dyck wrote:
>>
>> +++ lib/Basic/TargetInfo.cpp	(working copy)
>> @@ -67,12 +67,63 @@
>>    case SignedInt:        return "int";
>>    case UnsignedInt:      return "unsigned int";
>>    case SignedLong:       return "long int";
>> -  case UnsignedLong:     return "long unsigned int";
>> +  case UnsignedLong:     return "unsigned long int";
>>    case SignedLongLong:   return "long long int";
>> -  case UnsignedLongLong: return "long long unsigned int";
>> +  case UnsignedLongLong: return "unsigned long long int";
>>    }
>>  }
>>
>> Is this needed or just cleanup?
>
> More of an unrelated fix. Neither "long unsigned int" nor "long long
> unsigned int" are listed among the acceptable type specifiers in  
> section
> 6.7.2 of C99 or section 7.1.5.2 of C++98. Clang obviously must handle
> them correctly, but I figured changing them could only improve
> portability.

This is ok, because of p2: "the type specifiers may occur in any  
order, possibly intermixed with the other declaration specifiers."

I'd prefer to keep exactly the same as what GCC produces, this makes  
diffing .i files easier.

>> Please propose a patch that just uses the new methods you
>> added to simplify InitPreprocessor without changing the other
>> behavior.  That will make it easier to review the patch and
>> it will be more obviously correct (and can thus be applied
>> without thinking) :)
>
> Is the attached patch what you have in mind? Or would you like to  
> see a
> more aggressive refactoring to remove all of the literal type names  
> and
> constant suffixes from InitPreprocessor.cpp?

This is a great start, applied as r85481.  Removing the rest would be  
even better :)

>
>> The changes to stdint.h are difficult to understand from the
>> patch, but look basically reasonable to me.  In the next
>> round of patches, I'll look closer.  Just to verify, on
>> boring targets where bytes are 8 bits, no extra definitions
>> will come out of it (other than the new WIDTH macros)?
>
> There aren't any additional user macros, but there are a few more
> implementation macros. Some are for the SIG_ATOMIC definitions, which
> were previously hard-coded as 32-bit integers. There are also some new
> SIGNED macros so stdint.h can define the correct limits for the types
> that can can be either signed or unsigned.

Ok.  Please send this as a follow on patch.

>> The testcases are very nice: please merge them together into
>> one test that uses multiple RUN lines and uses a different
>> -check-prefix to FileCheck.  Also, the RUN lines don't need
>> to fit into 80 columns, please use "clang-cc -E ... |
>> FileCheck %s" instead of using %t.
>
> How about two tests: one to test stdint.h and the other to test basic
> preprocessor initialization? That is what is in the attached patch,
> anyways.

Looks great to me, applied as r85482, thanks!

> The patch corresponds to the current stdint.h, so it can be applied
> safely now, with (or without) the InitPreprocessor.cpp changes in the
> other patch. This will provide a baseline for evaluating further  
> changes
> to stdint.h and InitPreprocessor.cpp.

Awesome, thanks Ken!

-Chris