patch for generalizing stdint for n-bit bytes

Clang Frontend
1

Hello Clangers,

The attached patch is the work of Ken Dyck which I merged and massaged a bit to work with the top of the tree (R84372). This patch generalizes some of the defines produced by the preprocessor in preparation for future patches to support of n-bit bytes.

Some other highlights include the addition of 40 new test cases that help better validate the preprocessor and target defines. It also gets rid of std::vector<char> in favor of llvm::raw_ostream to create defines.

My hope is that someone with commit authority can look these changes and approve them or provide feedback for any additional mods work that is required for them to be accepted.

Since the patch is a bit large (thanks to all of the new tests!), I am including a custom htmlized view of the patch. (You might find it easier to grok the changes with...)

Thanks a bunch,

Ray

stdint-generalize-with-tests.patch (169 KB)

stdint-generalize-with-tests.patch.html (300 KB)

2

This patch generalizes some of the defines produced by the
preprocessor in preparation for future patches to support of
n-bit bytes.

Specifically, it adds support to stdint.h for all mutiple-of-8 integer
widths up to 64 bits, where it previously only supported 8-, 16-, 32-
and 64-bit widths.

Some other highlights include the addition of 40 new test
cases that help better validate the preprocessor and target
defines. It also gets rid of std::vector<char> in favor of
llvm::raw_ostream to create defines.

If it helps, I'd be happy to separate the new test cases and the
raw_ostream conversion into separate patches.

-Ken

3

Hi Ken,

Yes, that would probably be very helpful!

- Daniel

4

Attached are patches for the stdint.h/InitPreprocessor.cpp modifications and some new tests that exercise preprocessor initialization.

I'll hold off on submitting a patch for the vector<char>-to-raw_ostream conversion in InitPreprocessor.cpp until the stdint.h patch is accepted/rejected, as there is too much overlap between the two to make separating them worthwhile.

-Ken

cpp-init-tests.r84633.patch (95.3 KB)

stdint-generalize.r84633.patch (34.9 KB)

5

Hi Ken,

This is very interesting work!

First, TargetInfo.h/cpp: these look great. I applied these as r except for this hunk:

+++ 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?

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) :slight_smile:

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)?

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.

A minor issue, please don't use "const TargetInfo& TI", please use "const TargetInfo &TI".

-Chris

6

> Attached are patches for the stdint.h/InitPreprocessor.cpp
> modifications and some new tests that exercise preprocessor
> initialization.

First, TargetInfo.h/cpp: these look great. I applied these
as r except for this hunk:

+++ 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.

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) :slight_smile:

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?

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.

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.

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.

Cheers,
-Ken

InitPreprocessor-simplify.r84758.patch (1.58 KB)

cpp-tests-pre-stdint-mods.r84758.patch (75.5 KB)

7

+++ 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) :slight_smile:

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 :slight_smile:

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

8

Patch attached.

InitPreprocessor-more-simplified.r86134.patch (6.23 KB)

9

Awesome, applied in r86177, thanks!

-chris