[PATCH] D120244: [clang][sema] Enable first-class bool support for C2x

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.
Herald added a project: All.

Thanks for working on this! It's worth noting that http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2935.pdf was what was adopted by WG14 a few weeks ago, so you should be checking against that one, not the initial version of the paper.
 It's also worth noting that your changes are partially covering http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2934.pdf which was adopted at the same meeting (setting `Opts.Bool` to true means you get `bool` and `true` and `false`); I would note that in the commit message, because I don't think it's worth trying to separate out `bool`, `true`, and `false` from one another in the implementation. But it is worth adding the deprecation warning to use of `_Bool` from that paper and removing the macro for `bool` in C2x mode from `<stdbool.h>` as part of these changes in this patch.

I think you're missing some test coverage for the preprocessor in the patch. e.g.,

  #if true != 1
  #error "oops"
  #endif
  
  #if false != 0
  #error "oops"
  #endif

And I don't see the changes to the <stdbool.h> header which is now obsolete in C2x.

Oh, and the change should be added to the release notes.



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3247
 
   // OpenCL and C++ both have bool, true, false keywords.
+  Opts.Bool = Opts.OpenCL || Opts.CPlusPlus || Opts.C2x;
----------------
Comment is now stale.


================
Comment at: clang/test/Sema/c2x-bool.c:1
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c2x -x c -fsyntax-only -verify %s
+
----------------
Is the triple necessary?


================
Comment at: clang/test/Sema/c2x-bool.c:14
+_Static_assert(false == (bool)+0);
+
+
----------------
You should also add a test to verify that the predefined constants are usable as an integer constant expression:
```
char c1[true]; // good
_Static_assert(sizeof(c1) == sizeof(char[1]));
char c2[false]; // zero-sized array declaration
```
(This covers the changes in 6.6.)




================
Comment at: clang/test/Sema/c2x-bool.c:15
+
+
+static void *f = false; // expected-warning {{to null from a constant boolean expression}}
----------------
Another test case that's needed is for the integer promotion rules:
```
_Static_assert(_Generic(+true, bool : 0, unsigned int : 0, signed int : 1, default : 0));

struct S {
  bool b : 1;
} s;
_Static_assert(_Generic(+s.b, bool : 0, unsigned int : 0, signed int : 1, default : 0));
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120244/new/

https://reviews.llvm.org/D120244