>> 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
One obstacle I found to simplifying the code is that the char type is
not included in the TargetInfo::IntTy enumeration, making it necessary
to always add a special case for characters. What is the rational for
excluding characters? Would there be any objection to adding a Char type
to the enum?
>> 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.
Okay. FYI, this is the plan I'll be working from for future patches:
1. A simple reordering of typedefs in stdint.h (and corresponding test
cases) in anticipation of the next patch, which defines the types in
order of decreasing width.
2. Generalization of stdint.h for non-power-of-2 widths, along with
necessary modifications to InitPreprocessor.cpp. Only minor changes to
test cases (where long is replaced by int in some typedefs on targets
where they are the same width).
3. Parameterization of SIG_ATOMIC
4. Parameterization of limits on types that can be either signed or
unsigned.
5. Replace use of vector<char> for the definition buffer with a
raw_ostream.
Does that seem reasonable to you?
-Ken
This is a great start, applied as r85481. Removing the rest would be
even better
One obstacle I found to simplifying the code is that the char type is
not included in the TargetInfo::IntTy enumeration, making it necessary
to always add a special case for characters. What is the rational for
excluding characters? Would there be any objection to adding a Char type
to the enum?
It probably was just not needed yet, so noone added it.
Ok. Please send this as a follow on patch.
Okay. FYI, this is the plan I'll be working from for future patches:
1. A simple reordering of typedefs in stdint.h (and corresponding test
cases) in anticipation of the next patch, which defines the types in
order of decreasing width.
2. Generalization of stdint.h for non-power-of-2 widths, along with
necessary modifications to InitPreprocessor.cpp. Only minor changes to
test cases (where long is replaced by int in some typedefs on targets
where they are the same width).
3. Parameterization of SIG_ATOMIC
4. Parameterization of limits on types that can be either signed or
unsigned.
5. Replace use of vector<char> for the definition buffer with a
raw_ostream.
Does that seem reasonable to you?
Sounds great! Thanks Ken,
-Chris
>> This is a great start, applied as r85481. Removing the
rest would be
>> even better
>
> One obstacle I found to simplifying the code is that the
char type is
> not included in the TargetInfo::IntTy enumeration, making
it necessary
> to always add a special case for characters. What is the
rational for
> excluding characters? Would there be any objection to adding a Char
> type to the enum?
It probably was just not needed yet, so noone added it.
Alright. If there aren't any objections, then, I'll add it and do a more
thorough scrub of InitPreprocessor.cpp.
> FYI, this is the plan I'll be working from for future patches:
>
> 1. A simple reordering of typedefs in stdint.h (and
corresponding test
> cases) in anticipation of the next patch, which defines the
types in
> order of decreasing width.
> 2. Generalization of stdint.h for non-power-of-2 widths, along with
> necessary modifications to InitPreprocessor.cpp. Only minor
changes to
> test cases (where long is replaced by int in some typedefs
on targets
> where they are the same width).
> 3. Parameterization of SIG_ATOMIC
> 4. Parameterization of limits on types that can be either signed or
> unsigned.
> 5. Replace use of vector<char> for the definition buffer with a
> raw_ostream.
Stage #1 patch attached. It is a simple reordering of the definitions in
stdint.h and introduces no new function changes.
-Ken
stdint-reorder.r85522.patch (25.7 KB)
On second thought, I'm going to leave it out. After tinkering with this
a bit, it didn't really buy us much. Besides, the standard excludes char
from the "standard integer types", which makes the current code closer
to the spec.
I still plan to clean up InitPreprocessor.cpp a bit more, though, which
leads me to another question.
TargetInfo allows you to override the type, width, and alignment of
several type definitions that are synonyms for the standard integer
types (i.e. wchar_t, char16, char32, intmax_t). It is possible, for
example, to specify int as 16-bits wide, define wchar_t as int, and
wchar_t as 32-bits wide. This is exactly the situation with MSP430
because it overrides the width of int but not of wchar_t, and it leads
to inconsistencies in limits.h and stdint.h.
Are there any cases where you'd want to specify a width or alignment for
one of these typedefs that is different from the type that it denotes?
If not, I propose that we remove the width and alignment members from
TargetInfo and calculate their values from the type that they denote.
-Ken
hi Ken,
Sorry for the delay, I got sucked into other things. Applied as r86062!
-Chris
Stage #2a attached. This adds definitions for types of 8-bit multiples
from 8 to 64 to stdint.h and rationalizes the selection of types for the
exact-width definitions in InitPreprocessor.cpp.
Only minimal changes have been made to the test cases:
- A 'signed' specifier is added to the type definitions where it was
unspecified before
- int32_t on PIC16 and MSP430 is now defined as 'long int' instead of
'long long' and int64_t on SystemZ is now 'long int' instead of 'long
long int'. These are all changes in types of equal width.
In stage #2b, I plan to parameterize the suffixes used by the
constant-generating macros in stdint.h, and use those macros to define
the limits of the integer types.
-Ken
stdint-8-bit-multiples.r86691.patch (34.6 KB)
Applied in r86976, very nice Ken!
-Chris
Looks great to me, committed as r86977!
-Chris