subject:"\[PATCH\] D116203\: \[clang\] adds unary type transformations as compiler built\-ins"

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

e137fb6fb85 
 should 
fix this issue. Thanks for your patience!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Derek Schuff via Phabricator via cfe-commits

dschuff added a comment.

This broke us in emscripten as well (building with trunk clang against a 
recent-but-not-trunk version of libcxx). I can test the fix if you want.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits

fdeazeve added a comment.

In D116203#3740843 , @cjdb wrote:

> I think there's a forward fix available, but since I can't repro locally, I'm 
> going to need to push it to main and observe how the tools fare.

I need to be AFK for a bit now, but I can test locally on my end if you upload 
the patch here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

I think there's a forward fix available, but since I can't repro locally, I'm 
going to need to push it to main and observe how the tools fare.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

@fdeazeve I haven't been able to repro this issue at all.

In D116203#3740092 , @fdeazeve wrote:

> In D116203#3740015 , @cjdb wrote:
>
>> In D116203#3739856 , @fdeazeve 
>> wrote:
>>
>>> It seems like non-lldb bots are also failing, if it makes it easier to 
>>> reproduce:
>>>  
>>> https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c
>>
>> Thanks for flagging this. Based on the fact it only seems to be 
>> `remove_reference_t`, I'm wondering if this is an issue with D131732 
>>  (which was merged immediately after 
>> D116203 ).
>
> I'm not sure, to expand on my previous message, I tested these two commits 
> which appear one right after the other in the log:
>
> - e9ef45635b77 
>  (14 
> hou..) cjdb@g.. [clang] adds unary type transformations as compiler built-ins
> - d2d77e050b32 
>  (15 
> hou..) Ting.W.. [PowerPC][Coroutines] Add tail-call check with call 
> information for coroutines
>
> The tests pass on `d2d77e050b32` and fail on `e9ef45635b77`

I haven't been able to repro yet. If you're able to test without

In D116203#3740227 , @aprantl wrote:

> @cjdb Would you mind reverting the patch until we figured out a solution to 
> unblock the CI?

Sure, go ahead. I'm not even able to get to a point where I can repro the 
issue, as I get the following:

  Command Output (stdout):
  --
  lldb version 16.0.0git (g...@github.com:llvm/llvm-project.git revision 
11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9)
clang revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
llvm revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
  Unable to load lldb extension module.  Possible reasons for this include:
1) LLDB was built with LLDB_ENABLE_PYTHON=0
2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should 
refer to
   the version of Python that LLDB built and linked against, and PYTHONPATH
   should contain the Lib directory for the same python distro, as well as 
the
   location of LLDB's site-packages folder.
3) A different version of Python than that which was built against is 
exported in
   the system's PATH environment variable, causing conflicts.
4) The executable '/tmp/lldb-breaks/bin/lldb' could not be found.  Please 
check 
   that it exists and is executable.
  
  --
  Command Output (stderr):
  --
  Traceback (most recent call last):
File "/home/cjdb/projects/llvm-bisect/lldb/test/API/dotest.py", line 7, in 

  lldbsuite.test.run_suite()
File 
"/home/cjdb/projects/llvm-bisect/lldb/packages/Python/lldbsuite/test/dotest.py",
 line 892, in run_suite
  import lldb
  ModuleNotFoundError: No module named 'lldb'

1 and 4 seem to be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Adrian Prantl via Phabricator via cfe-commits

aprantl added a comment.

@cjdb Would you mind reverting the patch until we figured out a solution to 
unblock the CI?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits

fdeazeve added a comment.

In D116203#3740015 , @cjdb wrote:

> In D116203#3739856 , @fdeazeve 
> wrote:
>
>> It seems like non-lldb bots are also failing, if it makes it easier to 
>> reproduce:
>>  
>> https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c
>
> Thanks for flagging this. Based on the fact it only seems to be 
> `remove_reference_t`, I'm wondering if this is an issue with D131732 
>  (which was merged immediately after 
> D116203 ).

I'm not sure, to expand on my previous message, I tested these two commits 
which appear one right after the other in the log:

- e9ef45635b77 
 (14 
hou..) cjdb@g.. [clang] adds unary type transformations as compiler built-ins
- d2d77e050b32 
 (15 
hou..) Ting.W.. [PowerPC][Coroutines] Add tail-call check with call information 
for coroutines

The tests pass on `d2d77e050b32` and fail on `e9ef45635b77`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

In D116203#3739856 , @fdeazeve wrote:

> It seems like non-lldb bots are also failing, if it makes it easier to 
> reproduce:
>  
> https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c

Thanks for flagging this. Based on the fact it only seems to be 
`remove_reference_t`, I'm wondering if this is an issue with D131732 
 (which was merged immediately after D116203 
).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits

fdeazeve added a comment.

It seems like non-lldb bots are also failing, if it makes it easier to 
reproduce:
 
https://green.lab.llvm.org/green/view/Clang/job/clang-stage1-RA/30837/consoleFull#-54352964249ba4694-19c4-4d7e-bec5-911270d8a58c


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits

fdeazeve added a comment.

Forgot to add the failure message:

  /usr/include/c++/v1/experimental/propagate_const:138:11: error: no template 
named 'remove_reference_t'; did you mean 'remove_reference'?
typedef remove_reference_t())> element_type;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-22 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits

fdeazeve added a comment.

I believe this patch might have broken LLDB bots 
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46269/

I've confirmed that d2d77e050b32 
 is good 
while e9ef45635b77 
 (this 
commit) breaks some tests.

To repro, a build of llvm+lldb+libcxx+libcxxapi and

  # inside build dir
  /bin/llvm-lit -v 
tools/lldb/test/API/lang/objc/modules-objc-property/TestModulesObjCProperty.py

Alternatively, running the `check-lldb` target should do it. But there is one 
other unrelated failure that might show up in that target


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-14 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

Reverted this (and follow-ups) in aacf1a9742f714dd432117d82d19a007289c3dee 
 for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-14 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added a comment.

This also broke building with GCC 9 on Ubuntu 20.04:

  [5/225] ASTNodeAPI.json
  FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json
  cd /home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling && 
/home/martin/code/llvm-project/llvm/build/bin/clang-ast-dump 
--skip-processing=0 -I 
/home/martin/code/llvm-project/llvm/build/lib/clang/16.0.0/include -I 
/home/martin/code/llvm-project/llvm/tools/clang/include -I 
/home/martin/code/llvm-project/llvm/build/tools/clang/include -I 
/home/martin/code/llvm-project/llvm/build/include -I 
/home/martin/code/llvm-project/llvm/include -I /usr/include/c++/9 -I 
/usr/include/x86_64-linux-gnu/c++/9 -I /usr/include/c++/9/backward -I 
/usr/lib/gcc/x86_64-linux-gnu/9/include -I /usr/local/include -I 
/usr/include/x86_64-linux-gnu -I /usr/include --json-output-path 
/home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling/ASTNodeAPI.json
  clang-ast-dump: ../tools/clang/lib/Sema/DeclSpec.cpp:833: bool 
clang::DeclSpec::SetTypeSpecType(clang::DeclSpec::TST, clang::SourceLocation, 
const char*&, unsigned int&, const clang::PrintingPolicy&): Assertion 
`!isDeclRep(T) && !isTypeRep(T) && !isExprRep(T) && "rep required for these 
type-spec kinds!"' failed.
  Aborted (core dumped)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-14 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

This breaks building on windows: http://45.33.8.238/win/64423/step_4.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, though I did have a nit about diagnostic wording that you can feel free 
to take or leave at your discretion. Thank you for this work, this was a very 
large patch with a whole lot of review comments, but I think what we ended up 
with is really great!




Comment at: clang/include/clang/Sema/DeclSpec.h:424-431
+  static bool isTransformTypeTrait(TST T) {
+constexpr TST Traits[] = {
+#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) TST_##Trait,
+#include "clang/AST/TransformTypeTraits.def"
+};
+
+return (T >= Traits[0] && T <= std::end(Traits)[-1]);

cjdb wrote:
> aaron.ballman wrote:
> > Errr, this isn't Python, so that `[-1]` terrifies me. It took me a good 
> > solid ten minutes to convince myself this was actually valid. Any interest 
> > in something along the lines of this form?
> I have many questions about the decisions I made in this snippet.
It was definitely a great way to wake up this morning. "Wait, what? WHAT?!?" 
followed by a bunch of furious standards reading. :-D



Comment at: clang/lib/AST/TypePrinter.cpp:1158
raw_ostream ) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }

cjdb wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > rsmith wrote:
> > > > Remove this line too. No point building an RAII scope with nothing in 
> > > > it.
> > > Can we get rid of this function entirely in that case?
> > I believe you can.
> It looks like other things also have this as just empty, so maybe it's best 
> to keep it?
That's fine by me!



Comment at: clang/test/SemaCXX/type-traits.cpp:3505
+  { using ExpectedError = __make_unsigned(unsigned _BitInt(1)); }
+  // expected-error@*:*{{'make_unsigned' is only compatible with non-bool 
integers and enum types, but was given 'unsigned _BitInt(1)'}}
+  { using ExpectedError = __make_unsigned(UnscopedBit); }

This diagnostic is a bit unfortunate because it was given a non-bool integer, 
so I don't know that users will know how to fix the issue from the text. I 
leave it to your discretion where to split that diagnostic up with a `%select` 
so that `_BitInt` is special or not; I don't think users will hit this 
diagnostic all that often, but I do have a little bit of a worry about template 
metaprogramming accidentally getting into this state.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-12 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb marked an inline comment as done.
cjdb added inline comments.



Comment at: clang/include/clang/Sema/DeclSpec.h:424-431
+  static bool isTransformTypeTrait(TST T) {
+constexpr TST Traits[] = {
+#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) TST_##Trait,
+#include "clang/AST/TransformTypeTraits.def"
+};
+
+return (T >= Traits[0] && T <= std::end(Traits)[-1]);

aaron.ballman wrote:
> Errr, this isn't Python, so that `[-1]` terrifies me. It took me a good solid 
> ten minutes to convince myself this was actually valid. Any interest in 
> something along the lines of this form?
I have many questions about the decisions I made in this snippet.



Comment at: clang/lib/AST/TypePrinter.cpp:1158
raw_ostream ) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }

aaron.ballman wrote:
> cjdb wrote:
> > rsmith wrote:
> > > Remove this line too. No point building an RAII scope with nothing in it.
> > Can we get rid of this function entirely in that case?
> I believe you can.
It looks like other things also have this as just empty, so maybe it's best to 
keep it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Pre-commit CI found build errors that should be addressed.




Comment at: clang/include/clang/AST/TransformTypeTraits.def:1-2
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.

Have to fix the formatting manually though.



Comment at: clang/include/clang/Sema/DeclSpec.h:424-431
+  static bool isTransformTypeTrait(TST T) {
+constexpr TST Traits[] = {
+#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) TST_##Trait,
+#include "clang/AST/TransformTypeTraits.def"
+};
+
+return (T >= Traits[0] && T <= std::end(Traits)[-1]);

Errr, this isn't Python, so that `[-1]` terrifies me. It took me a good solid 
ten minutes to convince myself this was actually valid. Any interest in 
something along the lines of this form?



Comment at: clang/lib/AST/TypePrinter.cpp:1158
raw_ostream ) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }

cjdb wrote:
> rsmith wrote:
> > Remove this line too. No point building an RAII scope with nothing in it.
> Can we get rid of this function entirely in that case?
I believe you can.



Comment at: clang/lib/Parse/ParseDecl.cpp:3475
+case tok::identifier:
+ParseIdentifier : {
   // This identifier can only be a typedef name if we haven't already seen





Comment at: clang/lib/Sema/SemaType.cpp:9267
+  constexpr auto UKind = UTTKind::RemovePointer;
+  // We don't want block pointers or ObjectiveC's id type
+  if (!BaseType->isAnyPointerType() || BaseType->isObjCIdType())





Comment at: clang/lib/Sema/SemaType.cpp:9305-9308
+  else if (const auto *AT = Context.getAsArrayType(BaseType))
+return AT->getElementType();
+  else
+return BaseType;





Comment at: clang/lib/Sema/SemaType.cpp:9350-9352
+if (auto BitInt = dyn_cast(Underlying)) {
+  return S.Context.getBitIntType(!IsMakeSigned, BitInt->getNumBits());
 }

An interesting test case for you to consider (both for enumeration underlying 
types as well as types in general): `signed _BitInt` requires at least two 
bits, so trying to do a make_signed on `_BitInt(1)` should be diagnosed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-11 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

Thanks for patiently reviewing! I'll do the libc++ stuff and a pilot run in 
some code. Hopefully can get it merged tomorrow :-)




Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1149-1150
+  default:
+assert(false && "passed in an unhandled type transformation built-in");
+llvm_unreachable("passed in an unhandled type transformation built-in");
+  }

rsmith wrote:
> rsmith wrote:
> > We don't need both of these. Just the `llvm_unreachable` would suffice.
> This comment still needs to be addressed.
Could've sworn I did this one, but it might've gotten mixed up in the D116280 
switcheroo.



Comment at: clang/lib/Parse/ParseExpr.cpp:1066-1067
 
+  REVERTIBLE_TYPE_TRAIT(__add_lvalue_reference);
+  REVERTIBLE_TYPE_TRAIT(__add_rvalue_reference);
   REVERTIBLE_TYPE_TRAIT(__is_abstract);

rsmith wrote:
> Just curious: why do we only handle six of the traits here, rather than all 
> of them?
Why indeed. Perhaps these were the original six I worked on and didn't come 
back to update this.



Comment at: clang/lib/Parse/ParseExpr.cpp:1750
+#undef TRANSFORM_TYPE_TRAIT_DEF
+if (NextToken().is(tok::less)) {
+  Tok.setKind(tok::identifier);

cjdb wrote:
> rsmith wrote:
> > cjdb wrote:
> > > rsmith wrote:
> > > > Here you're checking for `trait<` and treating it as an identifier; 
> > > > elsewhere you're checking for `trait(` and treating it as a builtin. Is 
> > > > there a reason to treat `trait` followed by a token other than `(` or 
> > > > `<` inconsistently?
> > > The parser needs to treat `trait<` as an identifier to accommodate 
> > > libstdc++'s usage of a few of these as alias templates. I've added a 
> > > comment to explain why in the code.
> > I agree we need to treat `trait<` as an identifier and `trait(` as the 
> > builtin. My question is, why do we have inconsistent treatment of the 
> > remaining cases (`trait` followed by any other token)? For example, 
> > `MaybeParseTypeTransformTypeSpecifier` treats `trait + 1` as an identifier. 
> > But this code treats it as the builtin name.
> > 
> > I think there are two choices that make the most sense, if they work:
> > 
> > 1) `trait(` is the builtin and any other utterance is an identifier, or
> > 2) `trait<` is an identifier, `using trait =` is an identifier, and any 
> > other utterance is the builtin.
> > 
> > I think I prefer option (2), given that it's defining the narrower special 
> > case. But all I'm really looking for here is a consistent story one way or 
> > the other, if it's feasible to have one.
> I'd like for it to be (2) as well, but based on how we do alias-declarations, 
> I'm concerned that will have performance implications for a rare occurrence.
You haven't countered this concern, so I'm going to close for now and will 
happily revisit if this one just slipped by unnoticed.



Comment at: clang/lib/Sema/SemaType.cpp:9375
+  });
+  assert(Result != Consider->end());
+

rsmith wrote:
> Can this fail for an enum whose underlying type is `_BitInt(N)` for some 
> unusual `N`?
Nice catch. This wasn't supposed to happen, but I ended up changing the logic 
so that `_BitInt` is considered here instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-08-08 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith accepted this revision.
rsmith added a comment.

A handful more comments, but I think we've basically converged here. I'm happy 
to take another look after you address these if you'd like (or you could ask 
someone else for a final pass), or for you to land this once you're happy.

Before landing, it'd be good to patch libc++ to use these intrinsics and run 
its testsuite to look for any unexpected behavior changes, if you've not 
already done so with this version of the patch.




Comment at: clang/lib/AST/ASTContext.cpp:10811
   case BuiltinType::Long:
+  case BuiltinType::ULong:
 return UnsignedLongTy;

I think we don't need this case any more.



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1149-1150
+  default:
+assert(false && "passed in an unhandled type transformation built-in");
+llvm_unreachable("passed in an unhandled type transformation built-in");
+  }

rsmith wrote:
> We don't need both of these. Just the `llvm_unreachable` would suffice.
This comment still needs to be addressed.



Comment at: clang/lib/Parse/ParseExpr.cpp:1066-1067
 
+  REVERTIBLE_TYPE_TRAIT(__add_lvalue_reference);
+  REVERTIBLE_TYPE_TRAIT(__add_rvalue_reference);
   REVERTIBLE_TYPE_TRAIT(__is_abstract);

Just curious: why do we only handle six of the traits here, rather than all of 
them?



Comment at: clang/lib/Sema/SemaType.cpp:9359
+ SourceLocation Loc) {
+  static const std::array SignedIntegers = {
+  , ,,

Remove the `static` -- it's not correct to use a `static` local for this. There 
can be multiple `Sema` instances, and this will pick the types from whichever 
one was used the first time this function was called.



Comment at: clang/lib/Sema/SemaType.cpp:9361-9365
+  ,   , };
+  static const std::array UnsignedIntegers = {
+  , ,
+  ,  ,
+  , };

We don't have an `__int128` on 32-bit targets. Maybe below you can form an 
`ArrayRef` and slice off the last element if `__int128` is unsupported?



Comment at: clang/lib/Sema/SemaType.cpp:9372
+  llvm::find_if(*Consider, [, ](const CanQual *T) {
+return S.Context.getTypeSize(BaseType) ==
+   S.Context.getTypeSize(T->getTypePtr());

Please only compute the size of `BaseType` once.



Comment at: clang/lib/Sema/SemaType.cpp:9375
+  });
+  assert(Result != Consider->end());
+

Can this fail for an enum whose underlying type is `_BitInt(N)` for some 
unusual `N`?



Comment at: clang/lib/Sema/SemaType.cpp:9392-9393
+   BaseType->isWideCharType() ||
+   (BaseType->isEnumeralType() &&
+!GetEnumUnderlyingType(*this, BaseType, {})->isBitIntType()));
 

I've been pondering the proper behavior for `make_signed` on `enum E : 
_BitInt(N) {}`. I think the standard wording here is broken (see my email to 
the lib reflector, subject "make_signed / make_unsigned and padding / extended 
integer types"), but I'm not certain what the right rule is. Let's say that 
always producing a `_BitInt` type in that case is good enough for now. :)



Comment at: clang/lib/Sema/SemaType.cpp:9407
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);
+  switch (UKind) {
+  case UnaryTransformType::EnumUnderlyingType:

Have you considered moving the calls to `getUnaryTransformType` into this 
function? Right now they're scattered among the `Builtin*` helper functions, 
and it's hard to be sure that every code path calls it. I think it'd be a lot 
simpler to create the `UnaryTransformType` wrapper once, in a single place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-07-30 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb marked 13 inline comments as done.
cjdb added inline comments.



Comment at: clang/lib/Parse/ParseExpr.cpp:1755-1756
+  Tok.setKind(tok::identifier);
+  Diag(Tok, diag::ext_keyword_as_ident)
+  << Tok.getIdentifierInfo()->getName() << 0;
+  goto ParseIdentifier;

rsmith wrote:
> Is it feasible to also produce this warning for the corresponding case in 
> `MaybeParseTypeTransformTypeSpecifier` / the callers of that function?
Will this risk sending out the warning twice, and if so, can the diagnostic 
engine handle that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-07-07 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.

Comment at: clang/lib/AST/ASTContext.cpp:10733-10736
   // For enums, get the underlying integer type of the enum, and let the 
general
   // integer type signchanging code handle it.
   if (const auto *ETy = T->getAs())
 T = ETy->getDecl()->getIntegerType();

This isn't the correct behavior for `make_unsigned`. We need to pick the 
integer type with the lowest rank with the same size as the enumeration (see 
[the corresponding 
wording](http://eel.is/c++draft/meta.trans#tab:meta.trans.sign-row-3-column-2-sentence-1)).
 For example, if the underlying type of `E` is `long` and `sizeof(long) == 
sizeof(int)`, then `make_unsigned` is required to produce `unsigned int` not 
`unsigned long`.

It might be that the best approach here is for `BuiltinChangeSignedness` to 
only call `getCorrespondingUnsignedType` when `T` is a signed or unsigned 
integral type (notably excluding `char16_t` and friends -- see next comment -- 
but including `_BitInt`), and otherwise for it to look through the signed / 
unsigned integer types for the first one with the same size as `T` and pick 
that.

Comment at: clang/lib/AST/ASTContext.cpp:10747
+  case BuiltinType::Char16:
 return UnsignedShortTy;
   case BuiltinType::Int:

Mapping `char16_t` -> `unsigned short` seems target-specific to me. If plain 
`unsigned char` is 16 bits wide, then `make_unsigned` should be 
`unsigned char`. Same for `char32_t`, `wchar_t`.

Comment at: clang/lib/AST/ASTContext.cpp:5863
   // FIXME: derive from "Target" ?
-  return WCharTy;
+  return IntTy;
 }

cjdb wrote:
> rsmith wrote:
> > This change seems surprising. Can you explain what's going on here?
> Motivated by `__make_signed(wchar_t)` previously returning `wchar_t`, and 
> that it was seemingly inconsistent with `getUnsignedWCharType` below.
It looks to me like this breaks the implementation of a GCC compatibility 
feature. In GCC 8 and before, `signed wchar_t` is [accepted and means 
`wchar_t`](https://godbolt.org/z/5af7n9bef) and similarly `unsigned wchar_t` is 
accepted and means `unsigned int`, and Clang provides that behavior here; with 
this change, `signed wchar_t` will resolve to `int` instead in Clang, deviating 
from GCC's behavior for this GCC compatibility feature.

I think I'd be happy to see that GCC compatibility feature removed entirely, 
especially given that GCC 9 onwards removed it, it's an error by default in 
Clang, and [essentially no-one seems to be using 
it](https://www.google.com/search?q=%22-Wno-signed-unsigned-wchar%22), but that 
should not be done in this patch, and we shouldn't change its behavior here 
either.

Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1149-1150
+  default:
+assert(false && "passed in an unhandled type transformation built-in");
+llvm_unreachable("passed in an unhandled type transformation built-in");
+  }

We don't need both of these. Just the `llvm_unreachable` would suffice.

Comment at: clang/lib/Parse/ParseExpr.cpp:1755-1756
+  Tok.setKind(tok::identifier);
+  Diag(Tok, diag::ext_keyword_as_ident)
+  << Tok.getIdentifierInfo()->getName() << 0;
+  goto ParseIdentifier;

Is it feasible to also produce this warning for the corresponding case in 
`MaybeParseTypeTransformTypeSpecifier` / the callers of that function?

Comment at: clang/lib/Sema/SemaType.cpp:9228
+QualType Sema::BuiltinAddPointer(QualType BaseType, SourceLocation Loc) {
+  DeclarationName EntityName(BaseType.getBaseTypeIdentifier());
+  QualType PointerToT =

The name that's wanted by `BuildPointerType` is the name of the pointer 
variable, not the name of the type the pointer points to. Eg, for `int &*p;` it 
wants to say "`p` declared as a pointer to a reference". Passing in the name of 
the base type will mean that `__add_pointer(class X&)` could produce 
diagnostics like "`X` declared as a pointer to a reference", which is nonsense. 
Fortunately, that diagnostic is impossible here because we never pass it a 
reference, so you should be able to just pass a null `DeclarationName` instead.

Comment at: clang/lib/Sema/SemaType.cpp:9233-9234
+  : BaseType;
+  return Context.getUnaryTransformType(BaseType, PointerToT,
+   UTTKind::AddPointer);
+}

`BuildPointerType` can fail and return a null `QualType`; you'll need to detect 
that and do something different here -- either return the error to the caller 
or fall back to the original type for error recovery.

Comment at: clang/lib/Sema/SemaType.cpp:9244-9246
+  Qualifiers Quals = Pointee.getQualifiers();
+  return Context.getUnaryTransformType(
+  BaseType, Context.getQualifiedType(Pointee, Quals), UKind);

No need to add `Pointee`'s qualifiers to

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-06-22 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:5863
   // FIXME: derive from "Target" ?
-  return WCharTy;
+  return IntTy;
 }

rsmith wrote:
> This change seems surprising. Can you explain what's going on here?
Motivated by `__make_signed(wchar_t)` previously returning `wchar_t`, and that 
it was seemingly inconsistent with `getUnsignedWCharType` below.



Comment at: clang/lib/AST/ASTContext.cpp:10712-10713
 QualType ASTContext::getCorrespondingUnsignedType(QualType T) const {
-  assert((T->hasSignedIntegerRepresentation() || T->isSignedFixedPointType()) 
&&
+  assert((T->hasSignedIntegerRepresentation() || T->isIntegerType() ||
+  T->isEnumeralType() || T->isSignedFixedPointType()) &&
  "Unexpected type");

rsmith wrote:
> If we now allow unsigned types to be passed in here, can we do so 
> consistently?
> 
> This seems to do the wrong thing for a vector of scoped enumeration type. Is 
> that a problem for the builtins?
I think not, given we're not worrying about vector types right now?



Comment at: clang/lib/AST/TypePrinter.cpp:1158
raw_ostream ) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }

rsmith wrote:
> Remove this line too. No point building an RAII scope with nothing in it.
Can we get rid of this function entirely in that case?



Comment at: clang/lib/Parse/ParseExpr.cpp:1750
+#undef TRANSFORM_TYPE_TRAIT_DEF
+if (NextToken().is(tok::less)) {
+  Tok.setKind(tok::identifier);

rsmith wrote:
> cjdb wrote:
> > rsmith wrote:
> > > Here you're checking for `trait<` and treating it as an identifier; 
> > > elsewhere you're checking for `trait(` and treating it as a builtin. Is 
> > > there a reason to treat `trait` followed by a token other than `(` or `<` 
> > > inconsistently?
> > The parser needs to treat `trait<` as an identifier to accommodate 
> > libstdc++'s usage of a few of these as alias templates. I've added a 
> > comment to explain why in the code.
> I agree we need to treat `trait<` as an identifier and `trait(` as the 
> builtin. My question is, why do we have inconsistent treatment of the 
> remaining cases (`trait` followed by any other token)? For example, 
> `MaybeParseTypeTransformTypeSpecifier` treats `trait + 1` as an identifier. 
> But this code treats it as the builtin name.
> 
> I think there are two choices that make the most sense, if they work:
> 
> 1) `trait(` is the builtin and any other utterance is an identifier, or
> 2) `trait<` is an identifier, `using trait =` is an identifier, and any other 
> utterance is the builtin.
> 
> I think I prefer option (2), given that it's defining the narrower special 
> case. But all I'm really looking for here is a consistent story one way or 
> the other, if it's feasible to have one.
I'd like for it to be (2) as well, but based on how we do alias-declarations, 
I'm concerned that will have performance implications for a rare occurrence.



Comment at: clang/lib/Sema/SemaType.cpp:9178
+  // in the same group of qualifiers as 'const' and 'volatile', we're extending
+  // '__decay(T)' so that it also removes '__restrict' in C++.
+  Quals.removeCVRQualifiers();

rsmith wrote:
> Why "in C++"? It looks like it does this in C too, which is probably 
> appropriate. However, this is a divergence from what `std::decay_t` does in 
> libc++ and libstdc++, where it does not remove `__restrict`.
That sentence is specifically calling out that `__restrict` is being removed in 
C++ mode too. Tightened up the wording. I'm partial to `__decay` removing 
`__restrict` because it would be what language decay does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-06-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:9265-9266
+  ? Context.getBitIntType(!IsMakeSigned, Context.getIntWidth(BaseType))
+  : Context.getIntTypeForBitwidth(Context.getIntWidth(BaseType),
+  IsMakeSigned);
+  Qualifiers Quals = Underlying.getQualifiers();

rsmith wrote:
> cjdb wrote:
> > rsmith wrote:
> > > This is wrong: if, say, `int` and `long` are the same bit-width, this 
> > > will give the same type for `__make_unsigned(int)` and 
> > > `__make_unsigned(long)`, where they are required to produce `unsigned 
> > > int` and `unsigned long` respectively.
> > > 
> > > Please look at `Context.getCorrespondingSignedType` and 
> > > `Context.getCorrespondingUnsignedType` to see how to do this properly; 
> > > you may be able to call those functions directly in some cases, but be 
> > > careful about the cases where we're required to return the lowest-rank 
> > > int type of the given size. Note that `getIntTypeForBitwidth` does *not* 
> > > do that; rather, it produces the *preferred* type of the given width, and 
> > > for WebAssembly and AVR it produces something other than the lowest-rank 
> > > type in practice in some cases.
> > This makes me very happy.
> The comment on this producing the wrong underlying type for enums in some 
> cases doesn't seem to be addressed. You need to produce the lowest-rank type 
> of the given size, which is not what `getIntTypeForBitwidth` does.
Specifically, the rule you need to implement for enums is that the resulting 
type "names the [un]signed integer type with smallest rank (6.8.5) for which 
sizeof(T) == sizeof(type)". So the first of `[un]signed char`, `[un]signed 
short`, `[un]signed int`, `[un]signed long`, `[un]signed long long` that is the 
same size as the enum, and if none of those work then an extended integer type.

For example, if `int` and `long` are the same size, then given `enum E : long 
{};`, `__make_unsigned(E)` is required to produce `unsigned int`, not `unsigned 
long`. This is not what `getIntTypeForBitwidth` does in general; rather, it 
answers the target-specific question of "which integer type would you like to 
be used for an integer that is N bits wide?".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-06-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.

Comment at: clang/lib/AST/ASTContext.cpp:5863
   // FIXME: derive from "Target" ?
-  return WCharTy;
+  return IntTy;
 }

This change seems surprising. Can you explain what's going on here?

Comment at: clang/lib/AST/ASTContext.cpp:10712-10713
 QualType ASTContext::getCorrespondingUnsignedType(QualType T) const {
-  assert((T->hasSignedIntegerRepresentation() || T->isSignedFixedPointType()) 
&&
+  assert((T->hasSignedIntegerRepresentation() || T->isIntegerType() ||
+  T->isEnumeralType() || T->isSignedFixedPointType()) &&
  "Unexpected type");

If we now allow unsigned types to be passed in here, can we do so consistently?

This seems to do the wrong thing for a vector of scoped enumeration type. Is 
that a problem for the builtins?

Comment at: clang/lib/AST/ASTContext.cpp:10731-10759
+  case BuiltinType::Char_U:
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+  case BuiltinType::UChar:
+  case BuiltinType::Char8:
 return UnsignedCharTy;
   case BuiltinType::Short:

(Continuation of suggested edits in comment a few lines below.)

Comment at: clang/lib/AST/ASTContext.cpp:10785-10786
 return SatUnsignedLongFractTy;
   default:
-llvm_unreachable("Unexpected signed integer or fixed point type");
+llvm_unreachable("Unexpected integer or signed fixed point type");
   }

Perhaps we can handle all the plain unsigned types (other than `wchar_t` and 
`char`, which are special) here? This would also cover the fixed-point types.

Comment at: clang/lib/AST/ASTContext.cpp:10791
 QualType ASTContext::getCorrespondingSignedType(QualType T) const {
-  assert((T->hasUnsignedIntegerRepresentation() ||
-  T->isUnsignedFixedPointType()) &&
+  assert((T->hasUnsignedIntegerRepresentation() || T->isIntegerType() ||
+  T->isEnumeralType() || T->isUnsignedFixedPointType()) &&

Similar comments for this function as previous one.

Comment at: clang/lib/AST/TypePrinter.cpp:1158
raw_ostream ) {
   IncludeStrongLifetimeRAII Strong(Policy);
 }

Remove this line too. No point building an RAII scope with nothing in it.

Comment at: clang/lib/Parse/ParseExpr.cpp:1750
+#undef TRANSFORM_TYPE_TRAIT_DEF
+if (NextToken().is(tok::less)) {
+  Tok.setKind(tok::identifier);

cjdb wrote:
> rsmith wrote:
> > Here you're checking for `trait<` and treating it as an identifier; 
> > elsewhere you're checking for `trait(` and treating it as a builtin. Is 
> > there a reason to treat `trait` followed by a token other than `(` or `<` 
> > inconsistently?
> The parser needs to treat `trait<` as an identifier to accommodate 
> libstdc++'s usage of a few of these as alias templates. I've added a comment 
> to explain why in the code.
I agree we need to treat `trait<` as an identifier and `trait(` as the builtin. 
My question is, why do we have inconsistent treatment of the remaining cases 
(`trait` followed by any other token)? For example, 
`MaybeParseTypeTransformTypeSpecifier` treats `trait + 1` as an identifier. But 
this code treats it as the builtin name.

I think there are two choices that make the most sense, if they work:

1) `trait(` is the builtin and any other utterance is an identifier, or
2) `trait<` is an identifier, `using trait =` is an identifier, and any other 
utterance is the builtin.

I think I prefer option (2), given that it's defining the narrower special 
case. But all I'm really looking for here is a consistent story one way or the 
other, if it's feasible to have one.

Comment at: clang/lib/Sema/SemaType.cpp:9160-9164
+  Qualifiers Quals = Pointee.getQualifiers();
+  Quals.removeAddressSpace();
+  return Context.getUnaryTransformType(
+  BaseType,
+  QualType(Pointee.getSplitUnqualifiedType().Ty, Quals.getAsOpaqueValue()),

You're splitting the type into unqualified type and quailfiers twice here, once 
in the call to `getQualifiers` and again in `getSplitUnqualifiedType`. It'd be 
better to only split it once.

Comment at: clang/lib/Sema/SemaType.cpp:9161
+  Qualifiers Quals = Pointee.getQualifiers();
+  Quals.removeAddressSpace();
+  return Context.getUnaryTransformType(

Do we really want to remove the address space here? The libc++ and libstdc++ 
implementations of the trait don't do that (https://godbolt.org/z/jqafYP6er) 
and it makes the behavior of `__remove_pointer` inconsistent with the behavior 
of `__add_pointer`. Can we just produce the pointee type intact, including all 
of its qualifiers?

Comment at: clang/lib/Sema/SemaType.cpp:9164
+  BaseType,
+  QualType(Pointee.getSplitUnqualifiedType().Ty, Quals.getAsOpaqueValue()),
+  UKind);

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-06-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/type-traits.cpp:3099-3100
+
+  { int a[T(__is_same(remove_pointer_t, int))]; }
+  { int a[T(__is_same(remove_pointer_t, 
S))]; }
+}

cjdb wrote:
> cjdb wrote:
> > rsmith wrote:
> > > It seems strange to remove the address space qualifier here, given that 
> > > this trait doesn't remove cv-qualifiers.
> > Does `int __attribute__((address_space(1)))` make sense as a type? I 
> > thought it had to be a pointee.
> Gentle ping.
> Does int __attribute__((address_space(1))) make sense as a type? I thought it 
> had to be a pointee.

It doesn't have to be on a pointer or a pointee type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-06-14 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added inline comments.



Comment at: clang/lib/Parse/ParseExpr.cpp:1750
+#undef TRANSFORM_TYPE_TRAIT_DEF
+if (NextToken().is(tok::less)) {
+  Tok.setKind(tok::identifier);

rsmith wrote:
> Here you're checking for `trait<` and treating it as an identifier; elsewhere 
> you're checking for `trait(` and treating it as a builtin. Is there a reason 
> to treat `trait` followed by a token other than `(` or `<` inconsistently?
The parser needs to treat `trait<` as an identifier to accommodate libstdc++'s 
usage of a few of these as alias templates. I've added a comment to explain why 
in the code.



Comment at: clang/lib/Sema/SemaType.cpp:9251
+  BaseType->isBooleanType()) {
+Diag(Loc, diag::err_make_signed_integral_only) << IsMakeSigned << BaseType;
+return QualType();

cjdb wrote:
> rsmith wrote:
> > Should we support vector types here?
> Is it a conforming extension for `make_signed_t __attribute__((ext_vector_type(2)))>` to work?
Gentle ping.



Comment at: clang/test/SemaCXX/type-traits.cpp:3099-3100
+
+  { int a[T(__is_same(remove_pointer_t, int))]; }
+  { int a[T(__is_same(remove_pointer_t, 
S))]; }
+}

cjdb wrote:
> rsmith wrote:
> > It seems strange to remove the address space qualifier here, given that 
> > this trait doesn't remove cv-qualifiers.
> Does `int __attribute__((address_space(1)))` make sense as a type? I thought 
> it had to be a pointee.
Gentle ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-05-13 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added inline comments.



Comment at: clang/test/SemaCXX/type-traits.cpp:3495-3496
+
+  { int a[T(__is_same(make_signed_t, int))]; }
+  { int a[T(__is_same(make_signed_t, int))]; }
+  { int a[T(__is_same(make_signed_t, const int))]; }

cjdb wrote:
> rsmith wrote:
> > It'd be useful to test enums with different underlying types. However, this 
> > test is not portable: if `short` and `int` are the same size, this is 
> > required to produce `short`, not `int`. It'd be good to have some test 
> > coverage of that quirk too. Perhaps this is easiest to see with a test like:
> > 
> > ```
> > enum E : long long {};
> > static_assert(__is_same(__make_signed(E), long));
> > ```
> > 
> > ... which should hold in cases where `long` and `long long` are the same 
> > size and are larger than `int`.
> I agree, but what about when `long` is smaller than `int`?
Bleh, this is impossible. What I meant to ask is what happens when `long` and 
`int` are the same size, I think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-05-13 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:4155
+#undef TRANSFORM_TYPE_TRAIT_DEF
+  if (!ParseTypeTransformTypeSpecifier(DS))
+goto ParseIdentifier;

rsmith wrote:
> The name signature you're using for this function is inconsistent with the 
> conventions in the rest of the parser: when a `Parser::Parse...` function 
> with a `bool` return type returns `true`, it means "I have failed and issued 
> a diagnostic". For "parse this if possible and return whether you did", we 
> usually use `Parser::MaybeParse...` (eg, `MaybeParseAttributes`).
> 
> Alternatively you could do the one-token lookahead here. If the `setKind` 
> call is here, it'll be a lot clearer why it makes sense to `goto 
> ParseIdentifier;`.
> 
> Also, for workarounds for a standard library issue, we normally include a `// 
> HACK:` comment explaining what we're doing and why.
Comment added, but I'd prefer to keep the function call so it keeps the logic 
self-contained and named.



Comment at: clang/lib/Sema/SemaType.cpp:9154
+  constexpr auto UKind = UTTKind::RemovePointer;
+  if (!BaseType->isPointerType() && !BaseType->isObjCObjectPointerType())
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);

rsmith wrote:
> This is `!BaseType->isAnyPointerType()`.
> 
> What about block pointers? I think this patch is doing the right thing here, 
> by saying that this only applies to pointers spelled with `*` (plain and 
> obj-c pointers) and not to pointers spelled with `^`, but it seems worth 
> calling out to ensure that I'm not the only one who's thought about it :)
Done, and added a test.



Comment at: clang/lib/Sema/SemaType.cpp:9251
+  BaseType->isBooleanType()) {
+Diag(Loc, diag::err_make_signed_integral_only) << IsMakeSigned << BaseType;
+return QualType();

rsmith wrote:
> Should we support vector types here?
Is it a conforming extension for `make_signed_t` to work?



Comment at: clang/lib/Sema/SemaType.cpp:9265-9266
+  ? Context.getBitIntType(!IsMakeSigned, Context.getIntWidth(BaseType))
+  : Context.getIntTypeForBitwidth(Context.getIntWidth(BaseType),
+  IsMakeSigned);
+  Qualifiers Quals = Underlying.getQualifiers();

rsmith wrote:
> This is wrong: if, say, `int` and `long` are the same bit-width, this will 
> give the same type for `__make_unsigned(int)` and `__make_unsigned(long)`, 
> where they are required to produce `unsigned int` and `unsigned long` 
> respectively.
> 
> Please look at `Context.getCorrespondingSignedType` and 
> `Context.getCorrespondingUnsignedType` to see how to do this properly; you 
> may be able to call those functions directly in some cases, but be careful 
> about the cases where we're required to return the lowest-rank int type of 
> the given size. Note that `getIntTypeForBitwidth` does *not* do that; rather, 
> it produces the *preferred* type of the given width, and for WebAssembly and 
> AVR it produces something other than the lowest-rank type in practice in some 
> cases.
This makes me very happy.



Comment at: clang/lib/Sema/SemaType.cpp:9267-9270
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.setCVRQualifiers(BaseType.getCVRQualifiers());
+  Underlying = QualType(Underlying.getSplitUnqualifiedType().Ty,
+Quals.getAsOpaqueValue());

rsmith wrote:
> As before, the opaque value is, well, opaque, and you shouldn't be using it 
> in this way. Also, you just created `Underlying` so you know it's 
> unqualified, so there's no need to ask for its qualifiers.
> 
> See suggested edit. Alternatively (#1), if you want to preserve all 
> qualifiers:
> 
> ```
> Underlying = Context.getQualifiedType(BaseType.getQualifiers());
> ```
> 
> Alternatively (#2) we could strictly follow the standard wording and preserve 
> only cv-qualifiers and not `restrict`. I think preserving all qualifiers is 
> more in line with the intent, but preserving only `const` and `volatile` is 
> probably the best match for what a C++ implementation of the type trait would 
> do. *shrug*
As with `decay`, I think it's best if we pretend `restrict` is a standard 
qualifier, either for everything or for nothing. However, I don't think that 
`restrict` is a valid contender here. Isn't it only applicable to pointers, 
which aren't transformed by this pair?



Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(

rsmith wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > What about things like type attributes (are those lost during decay)?
> > According to https://eel.is/c++draft/tab:meta.trans.other, no.
> Type attributes are non-standard, so we can't answer this question by

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-25 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.

Comment at: clang/include/clang/AST/TransformTypeTraits.def:27
+TRANSFORM_TYPE_TRAIT_DEF(RemoveVolatile, remove_volatile)
+TRANSFORM_TYPE_TRAIT_DEF(EnumUnderlyingType, underlying_type)

This file should undef the `TRANSFORM_TYPE_TRAIT_DEF` macro at the end, like 
our other `.def` files do. Then you can remove all of the `#undef`s elsewhere 
in this patch.

Comment at: clang/include/clang/AST/Type.h:4567-4582
+EnumUnderlyingType,
+AddLvalueReference,
+AddPointer,
+AddRvalueReference,
+Decay,
+MakeSigned,
+MakeUnsigned,

Consider using the `.def` file here.

Comment at: clang/include/clang/AST/Type.h:6528
+  (void)IsCPlusPlus;
+  assert(IsCPlusPlus && "Referenceable types only make sense in C++");
+  const Type  = **this;

I think the existence of the `IsCPlusPlus` flag may have the opposite effect of 
the one that's intended:

* Without this flag, looking only at the declaration of this function, I would 
assume you're not supposed to call this function except in C++ mode.
* With this flag, looking only at the declaration of this function, I would 
assume that passing in `IsCPlusPlus = false` would cause the function to always 
return `false` (because there are no reference types).

It might be better to define "referenceable" as "function type with no 
ref-qualifier nor cv-qualifier, object type, or reference type", and just 
accept calls to `isReferenceable` across all language modes.

Comment at: clang/include/clang/AST/Type.h:6532-6537
+  if (!Self.isFunctionType())
+return false;
+
+  const auto *F = Self.getAs();
+  assert(F != nullptr);
+  return F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None;

It's better not to test whether the type is a function type (walking past sugar 
etc) more than once.

Comment at: clang/include/clang/Sema/DeclSpec.h:340
   /*TSS*/unsigned TypeSpecSign : 2;
-  /*TST*/unsigned TypeSpecType : 6;
+  /*TST*/ unsigned TypeSpecType : 7;
   unsigned TypeAltiVecVector : 1;

Please keep the formatting consistent here, even if you need to override 
clang-format.

Comment at: clang/include/clang/Sema/DeclSpec.h:407
   static bool isTypeRep(TST T) {
-return (T == TST_typename || T == TST_typeofType ||
-T == TST_underlyingType || T == TST_atomic);
+static const llvm::SmallVector validTSTs = {
+TST_atomic,

aaron.ballman wrote:
> Should we find a way we can drop the `19` here so that this doesn't become 
> incorrect when someone adds anything to `TransformTypeTraits.def`?
It'd be nice to just compare `T` against the least and greatest trait value -- 
this linear scan seems more expensive than we might want. (And sure, we don't 
care about efficiency given that this function is only used by asserts, but I 
don't think we should assume it'll remain that way.)

Eg, something like:

```
constexpr TST Traits[] = {
#define TRANSFORM_TYPE_TRAIT_DEF(_, Trait) TST_##Trait,
#include "clang/AST/TransformTypeTraits.def"
#undef TRANSFORM_TYPE_TRAIT_DEF
};
static_assert(std::is_sorted(std::begin(Traits), std::end(Traits)));
return T == TST_atomic || T == TST_typename || T == TST_typeofType || (T >= 
Traits[0] && T <= std::end(Traits)[-1]);
```

Comment at: clang/lib/AST/ItaniumMangle.cpp:3962
   if (T->isDependentType()) {
-Out << 'U';
+Out << "Uu";

This should just be `u`, not `Uu`.

Comment at: clang/lib/AST/ItaniumMangle.cpp:3976

   mangleType(T->getBaseType());
 }

You're missing the `I`...`E` around this argument.

Comment at: clang/lib/Parse/ParseDecl.cpp:3419-3422
 case tok::kw___super:
 case tok::kw_decltype:
 case tok::identifier: {
+ParseIdentifier:

Our normal convention is to put the `{` after the last label in a sequence of 
labels.

Comment at: clang/lib/Parse/ParseDecl.cpp:4155
+#undef TRANSFORM_TYPE_TRAIT_DEF
+  if (!ParseTypeTransformTypeSpecifier(DS))
+goto ParseIdentifier;

The name signature you're using for this function is inconsistent with the 
conventions in the rest of the parser: when a `Parser::Parse...` function with 
a `bool` return type returns `true`, it means "I have failed and issued a 
diagnostic". For "parse this if possible and return whether you did", we 
usually use `Parser::MaybeParse...` (eg, `MaybeParseAttributes`).

Alternatively you could do the one-token lookahead here. If the `setKind` call 
is here, it'll be a lot clearer why it makes sense to `goto ParseIdentifier;`.

Also, for workarounds for a standard library issue, we normally include a `// 
HACK:` comment explaining what we're doing and why.

Comment at: clang/lib/Parse/ParseExpr.cpp:1041-1044
   case

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-20 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-18 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Generally looking good to me, mostly just drive-by commentary at this point.




Comment at: clang/include/clang/Sema/DeclSpec.h:407
   static bool isTypeRep(TST T) {
-return (T == TST_typename || T == TST_typeofType ||
-T == TST_underlyingType || T == TST_atomic);
+static const llvm::SmallVector validTSTs = {
+TST_atomic,

Should we find a way we can drop the `19` here so that this doesn't become 
incorrect when someone adds anything to `TransformTypeTraits.def`?



Comment at: clang/lib/Parse/ParseDecl.cpp:3421-3422
 case tok::kw_decltype:
 case tok::identifier: {
+ParseIdentifier:
   // This identifier can only be a typedef name if we haven't already seen

I don't know why this offends my sensibilities, but it does. It seems like the 
label should be with the other (case) labels to my lizard brain, but the 
current way is also correct.



Comment at: clang/lib/Parse/ParseExpr.cpp:1044
// constant: enumeration-constant
+  ParseIdentifier:
 // Turn a potentially qualified name into a annot_typename or

Well, at least my sensibilities are consistent about pearl clutching, as this 
one also jumped out at me. :-D



Comment at: clang/lib/Parse/ParseExpr.cpp:1784
   default:
+  ExpectedExpression:
 NotCastExpr = true;

And yet this one didn't offend my sensibilities.  It's the curly braces that's 
doing it. :-D



Comment at: clang/lib/Parse/ParseStmt.cpp:186
   case tok::identifier: {
+  ParseIdentifier:
 Token Next = NextToken();

Gahh!

(Also, I'm a bit concerned that this feature requires so many `goto`s. Each one 
is reasonable in isolation, but we're adding a fair amount of total parsing 
complexity with those. That said, I don't have a better solution to suggest.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-16 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb marked an inline comment as done.
cjdb added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:9113
+  BaseType.isReferenceable() || BaseType->isVoidType()
+  ? BuildPointerType(BaseType.getNonReferenceType(), Loc, EntityName)
+  : BaseType;

cjdb wrote:
> erichkeane wrote:
> > Do we at any point want this builtin to be address-space aware?  
> I'm not against this functionality, but there isn't (currently) motivation 
> for it. WDYT about `__add_pointer(T, address_space)`? Do we want this to be a 
> separate builtin (e.g. `__add_pointer_with_address_space(T, address_space)`)?
I don't think handling address spaces is worth the trouble: we can always do 
`__add_pointer(int __attribute__((address_space(1` if desired (I added two 
tests to confirm this). I did need to fix `__remove_pointer` so that it also 
removes address spaces, so thank you for flagging this :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-11 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

The patch description doesn't match the patch: we have `__remove_cv` in the 
description but `__remove_cv_qualifiers` in the patch. It seems unfortunate to 
me to use a long-term bad name for our trait to  work around a (likely) 
short-term problem with a specific version of libstdc++. Perhaps we could 
demote the `__remove_cv` keyword to a plain identifier if it's not followed by 
a `(` instead (we've done similar things for other trait keywords that have 
conflicted with libstdc++); would that be enough to resolve the incompatibility?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-11 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3957
 
 switch (T->getUTTKind()) {
   case UnaryTransformType::EnumUnderlyingType:

I'd prefer that you restructure this to first compute a `StringRef` 
corresponding to the name of the trait and then use `Out << 'u' << Name.size() 
<< Name`. (That'd make the miscounting error that a previous version of this 
patch had with `__remove_pointer` impossible.)



Comment at: clang/lib/AST/JSONNodeDumper.cpp:670
+  case UnaryTransformType::AddLvalueReference:
+JOS.attribute("transformedKind", "add_lvalue_reference");
+break;

`__underlying_type` has a property called `transformKind`; here you're using 
the name `transformedKind` instead. I think the non-`ed` name makes more sense.



Comment at: clang/lib/AST/TextNodeDumper.cpp:1553
+  case UnaryTransformType::AddLvalueReference:
+OS << " add_lvalue_reference";
+break;

This is the third time we've hardcoded a mapping between enumerators and names 
(and it looks like there are more to follow). That seems enough to be worth 
adding a .def file to avoid repeating this mapping.



Comment at: clang/lib/Parse/ParseDecl.cpp:4149-4168
 case tok::kw___underlying_type:
   ParseUnderlyingTypeSpecifier(DS);
   continue;
 
+case tok::kw___add_lvalue_reference:
+case tok::kw___add_pointer:
+case tok::kw___add_rvalue_reference:

Do we need different handling for `__underlying_type` versus the other unary 
type transforms here?



Comment at: clang/lib/Parse/ParseDecl.cpp:4287
+  default:
+llvm_unreachable(__FILE__
+ "passed in an unhandled type transformation built-in");

If we want a file name in `llvm_unreachable` and it's not providing one, I 
think that's something we should add centrally rather than in this one usage -- 
I'd prefer that you not pass `__FILE__` here and instead produce a separate 
patch to extend `llvm_unreachable` if needed. (Also you seem to be missing any 
whitespace in the resulting string between the filename and the start of the 
message.)



Comment at: clang/lib/Parse/ParseDecl.cpp:4297
+  if (T.expectAndConsume(diag::err_expected_lparen_after,
+ "type transformation builtin", tok::r_paren))
+return;

Do not pass English-language text fragments to diagnostics -- we aim for our 
diagnostics to (at least in principle) be translatable. Instead, add a new 
diagnostic for this situation (or perhaps just use `diag::err_expected, 
tok::l_paren`).



Comment at: clang/lib/Parse/ParseDecl.cpp:4317
+  }
+  DS.setTypeofParensRange(T.getRange());
+}

Can we give this a better name, given that it's not used for only `typeof`? 
Maybe something like `setTypeArgumentRange`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-11 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/include/clang/AST/Type.h:6535
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

You can turn this into an assert that `F` is not null -- C++ doesn't have the 
notion of functions without a prototype.



Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

cjdb wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > aaron.ballman wrote:
> > > > A function without a prototype is referenceable? (This is more of a 
> > > > "should this predicate do anything in C?" kind of question, I suppose.)
> > > Does C++ have a notion of non-prototyped functions? I don't think it 
> > > does? As such, I'm struggling to model this in a way that makes sense :(
> > > 
> > > Maybe that's evidence for NAD, maybe it isn't :shrug:
> > C++ doesn't have the notion of a function without a prototype (thankfully).
> > 
> > Should this function assert that it's not called in C mode at all? I don't 
> > think asking the question makes sense in C.
> I made a change in that this asserts when not in C++ mode.
The assert is fine by me, but I think you're going to need to add a 
`(void)IsCPlusPlus;` to the function (or a `[[maybe_unused]]` to the parameter) 
otherwise non-asserts builds may start to diagnose the unused parameter.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

cjdb wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > aaron.ballman wrote:
> > > > This can be reformatted, I believe, but did you look around to see if 
> > > > an existing diagnostic would suffice?
> > > Do you have any tips on how to narrow my search? I don't really want to 
> > > //read// 11K lines of diagnostics.
> > I usually search for "typecheck" for type checking diagnostics. One that 
> > looks close is:
> > ```
> > def err_pragma_loop_invalid_argument_type : Error<
> >   "invalid argument of type %0; expected an integer type">;
> > ```
> > that could likely be changed to something along the lines of:
> > ```
> > def err_invalid_argument_type : Error<
> >   "invalid argument of type %0; expected an integer type %select{|other 
> > than 'bool'}1">;
> > ```
> > (I think enum types are always integer types and so they may not need to be 
> > called out in the diagnostic.)
> I'm not particularly fond of this one because it doesn't call out the 
> builtin's user-facing name. I suppose we could do this, but I'm not sure 
> where to draw the line:
> 
> ```
> def err_invalid_argument_type : Error<
>   "invalid argument %select{|for 'make_%select{signed|unsigned}2'}1, with 
> type %0; expected an integer type %select{|other than 'bool'}3">;
> ```
> 
> This means that users who misuse `std::make_[un]signed` will get a nice 
> diagnostic instead of one targeting `__make_[un]signed`, which helps them 
> work out where they can fix their code.
Eh, may as well go with a dedicated diagnostic at that point, that seems like 
it's making the diagnostic far less general.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
 break;
+  case UnaryTransformType::AddConst:
+Out << "2ac";

rsmith wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > Are these the suggested manglings from the Itanium mangling document, or 
> > > something you invented yourself?
> > > 
> > > Should there be corresponding Microsoft manglings or are those already 
> > > handled magically?
> > > 
> > > Also, test coverage for the manglings?
> > I copied the mangling from D67052 and then inferred for what's missing over 
> > there. I'll consult the Itanium mangling doc to ensure that they're 
> > correct. How can I check the corresponding Microsoft manglings are handled?
> This is mangling the trait as a vendor-specific type *qualifier*, so 
> `__add_lvalue_reference(T)` will demangle as `alref T` instead. The 
> underlying_type trait probably does this because it predates the Itanium ABI 
> having a mangling form for a vendor-specific type *specifier* with arguments. 
> Also, if we want this to demangle properly, we should follow the ABI 
> specification's recommendations and use the actual source name of the 
> builtin. So I think the ideal manglings (other than being kinda long, which 
> probably doesn't matter given that these are unlikely to show up in actual 
> mangled names) would be things like:
> 
> `u13__add_pointerI` ... inner type mangling ... `E`
> 
> It would probably be OK to change the mangling for `__enum_underlying_type` 
> to this form

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-09 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

I'm totally fine with a specifier that removes all qualifiers; I just think we 
can't use it for `std::remove_cv`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D116203#3436572 , @cjdb wrote:

> Having `__remove_cv` do more than it's advertised to do doesn't sound like a 
> great idea to me. Both libc++ 
> 
>  and libstdc++ 
> 
>  define `std::remove_cv` without extensions, and I think it would be 
> surprising for `__remove_cv` to remove anything else.
>
> I'm not against adding `__remove_restrict`, `__remove_qualifiers` (although 
> "qualifier" could imply ref-qualifiers too?), etc.. I suppose that in the 
> rare case someone wants to remove `volatile restrict` and keep `const&`, it's 
> possible to do `__add_const(__remove_qualifiers(T)&)`.

Sorry, I was really unclear it seems. I was recommending we end up with:

`__remove_const`
`__remove_volatile`
`__remove_restrict`
`__remove_cv`  // Removes just `const` and `volatile`
`__remove_qualifiers` // Removes all qualifiers

and if we someday find a use for adding `__remove_cvr` we can add it (but I 
would be surprised if the need was that great).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-07 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

In D116203#3435729 , @aaron.ballman 
wrote:

> In D116203#3434761 , @rjmccall 
> wrote:
>
>> In D116203#3434515 , @cjdb wrote:
>>
>>> In D116203#3431612 , @rjmccall 
>>> wrote:
>>>
 In D116203#3430332 , 
 @aaron.ballman wrote:

> In D116203#3425512 , @cjdb 
> wrote:
>
>> I've noticed that libstdc++ has `using __remove_cv = typename 
>> remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
>> `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
>> Is there a way we can work around this, or should we just rename 
>> `__remove_cv` and friends to something else?
>
> You could work around it by taking note that you're in a libstdc++ system 
> header and do a special dance, but because these are in the 
> implementation's namespace, I think it's probably kinder for everyone to 
> just pick a different name.
>>>
>>> I was hoping we could do something similar to `struct __remove_cv` which 
>>> would issue a warning?
>>>
> If you wanted to be especially mean, you could go with `__remove_cvr`, 
> but I'd suggest `__remove_cv_qualifiers` instead. However, what about 
> `restrict` qualifiers? We support them in C++: 
> https://godbolt.org/z/11EPefhjf

 Along with a fair number of other vendor qualifiers, yeah.  I think you 
 have to make a policy decision about whether the intent of 
 `std::remove_cv` is really to just remove CV qualifiers or to produce an 
 unqualified type (at the outermost level).  The former is probably more 
 defensible, even if it makes the transform less useful in the presence of 
 extended qualifiers.
>>>
>>> I'm partial to `std::remove_cv` being faithful to its name, unless existing 
>>> implementations do something else already. I don't mind adding support for 
>>> the other stuff, but if there's more than just add/remove `restrict`, we're 
>>> going to have a combinatorial explosion for removes. Is there an alternate 
>>> way we can approach this?
>>> Possibly:
>>>
>>>   template
>>>   using remove_const_t = __remove_qualifiers(T, const);
>>>   
>>>   
>>>   template
>>>   using remove_reference_t = __remove_qualifiers(T, &, &&);
>>>   
>>>   template
>>>   using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, 
>>> &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t
>>
>> I don't think it's worth adding that parsing complexity for a builtin that 
>> we expect to only be used in system headers.  Let's just remove `const` and 
>> `volatile` and leave other qualifiers in place.
>
> I come down on the opposite side of the fence and think it should remove all 
> qualifiers (or that should be an interface we support in addition to removing 
> just cv qualifiers). WG14 adopted 
> https://www9.open-std.org/jtc1/sc22/wg14/www/docs/n2927.htm into C23 with a 
> `remove_quals` function which removes *all* qualifiers (including `_Atomic`), 
> as an example. But even in C++ mode, I fail to see why we wouldn't want 
>  interface for stripping `__restrict` the same as `const` or `volatile` 
> along with some interface for stripping all qualifiers period (I don't see a 
> huge need for a `__remove_cvr` if we have the ability to remove all 
> qualifiers, so I don't think we need the combinatorial explosion or a complex 
> interface). Basically -- if we have extensions like `__restrict` or _Atomic, 
> we should fully support them rather than halfway do it.

Having `__remove_cv` do more than it's advertised to do doesn't sound like a 
great idea to me. Both libc++ 

 and libstdc++ 

 define `std::remove_cv` without extensions, and I think it would be surprising 
for `__remove_cv` to remove anything else.

I'm not against adding `__remove_restrict`, `__remove_qualifiers` (although 
"qualifier" could imply ref-qualifiers too?), etc.. I suppose that in the rare 
case someone wants to remove `volatile restrict` and keep `const&`, it's 
possible to do `__add_const(__remove_qualifiers(T)&)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D116203#3434761 , @rjmccall wrote:

> In D116203#3434515 , @cjdb wrote:
>
>> In D116203#3431612 , @rjmccall 
>> wrote:
>>
>>> In D116203#3430332 , 
>>> @aaron.ballman wrote:
>>>
 In D116203#3425512 , @cjdb wrote:

> I've noticed that libstdc++ has `using __remove_cv = typename 
> remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
> `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
> Is there a way we can work around this, or should we just rename 
> `__remove_cv` and friends to something else?

 You could work around it by taking note that you're in a libstdc++ system 
 header and do a special dance, but because these are in the 
 implementation's namespace, I think it's probably kinder for everyone to 
 just pick a different name.
>>
>> I was hoping we could do something similar to `struct __remove_cv` which 
>> would issue a warning?
>>
 If you wanted to be especially mean, you could go with `__remove_cvr`, but 
 I'd suggest `__remove_cv_qualifiers` instead. However, what about 
 `restrict` qualifiers? We support them in C++: 
 https://godbolt.org/z/11EPefhjf
>>>
>>> Along with a fair number of other vendor qualifiers, yeah.  I think you 
>>> have to make a policy decision about whether the intent of `std::remove_cv` 
>>> is really to just remove CV qualifiers or to produce an unqualified type 
>>> (at the outermost level).  The former is probably more defensible, even if 
>>> it makes the transform less useful in the presence of extended qualifiers.
>>
>> I'm partial to `std::remove_cv` being faithful to its name, unless existing 
>> implementations do something else already. I don't mind adding support for 
>> the other stuff, but if there's more than just add/remove `restrict`, we're 
>> going to have a combinatorial explosion for removes. Is there an alternate 
>> way we can approach this?
>> Possibly:
>>
>>   template
>>   using remove_const_t = __remove_qualifiers(T, const);
>>   
>>   
>>   template
>>   using remove_reference_t = __remove_qualifiers(T, &, &&);
>>   
>>   template
>>   using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, 
>> &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t
>
> I don't think it's worth adding that parsing complexity for a builtin that we 
> expect to only be used in system headers.  Let's just remove `const` and 
> `volatile` and leave other qualifiers in place.

I come down on the opposite side of the fence and think it should remove all 
qualifiers (or that should be an interface we support in addition to removing 
just cv qualifiers). WG14 adopted 
https://www9.open-std.org/jtc1/sc22/wg14/www/docs/n2927.htm into C23 with a 
`remove_quals` function which removes *all* qualifiers (including `_Atomic`), 
as an example. But even in C++ mode, I fail to see why we wouldn't want  
interface for stripping `__restrict` the same as `const` or `volatile` along 
with some interface for stripping all qualifiers period (I don't see a huge 
need for a `__remove_cvr` if we have the ability to remove all qualifiers, so I 
don't think we need the combinatorial explosion or a complex interface). 
Basically -- if we have extensions like `__restrict` or _Atomic, we should 
fully support them rather than halfway do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-06 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
 break;
+  case UnaryTransformType::AddConst:
+Out << "2ac";

cjdb wrote:
> aaron.ballman wrote:
> > Are these the suggested manglings from the Itanium mangling document, or 
> > something you invented yourself?
> > 
> > Should there be corresponding Microsoft manglings or are those already 
> > handled magically?
> > 
> > Also, test coverage for the manglings?
> I copied the mangling from D67052 and then inferred for what's missing over 
> there. I'll consult the Itanium mangling doc to ensure that they're correct. 
> How can I check the corresponding Microsoft manglings are handled?
This is mangling the trait as a vendor-specific type *qualifier*, so 
`__add_lvalue_reference(T)` will demangle as `alref T` instead. The 
underlying_type trait probably does this because it predates the Itanium ABI 
having a mangling form for a vendor-specific type *specifier* with arguments. 
Also, if we want this to demangle properly, we should follow the ABI 
specification's recommendations and use the actual source name of the builtin. 
So I think the ideal manglings (other than being kinda long, which probably 
doesn't matter given that these are unlikely to show up in actual mangled 
names) would be things like:

`u13__add_pointerI` ... inner type mangling ... `E`

It would probably be OK to change the mangling for `__enum_underlying_type` to 
this form too. I doubt that's made it into real mangled names in the wild, but 
if you want to add `-fclang-abi-compat` support for that, it wouldn't hurt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-06 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

In D116203#3434515 , @cjdb wrote:

> In D116203#3431612 , @rjmccall 
> wrote:
>
>> In D116203#3430332 , 
>> @aaron.ballman wrote:
>>
>>> In D116203#3425512 , @cjdb wrote:
>>>
 I've noticed that libstdc++ has `using __remove_cv = typename 
 remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
 `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
 Is there a way we can work around this, or should we just rename 
 `__remove_cv` and friends to something else?
>>>
>>> You could work around it by taking note that you're in a libstdc++ system 
>>> header and do a special dance, but because these are in the 
>>> implementation's namespace, I think it's probably kinder for everyone to 
>>> just pick a different name.
>
> I was hoping we could do something similar to `struct __remove_cv` which 
> would issue a warning?
>
>>> If you wanted to be especially mean, you could go with `__remove_cvr`, but 
>>> I'd suggest `__remove_cv_qualifiers` instead. However, what about 
>>> `restrict` qualifiers? We support them in C++: 
>>> https://godbolt.org/z/11EPefhjf
>>
>> Along with a fair number of other vendor qualifiers, yeah.  I think you have 
>> to make a policy decision about whether the intent of `std::remove_cv` is 
>> really to just remove CV qualifiers or to produce an unqualified type (at 
>> the outermost level).  The former is probably more defensible, even if it 
>> makes the transform less useful in the presence of extended qualifiers.
>
> I'm partial to `std::remove_cv` being faithful to its name, unless existing 
> implementations do something else already. I don't mind adding support for 
> the other stuff, but if there's more than just add/remove `restrict`, we're 
> going to have a combinatorial explosion for removes. Is there an alternate 
> way we can approach this?
> Possibly:
>
>   template
>   using remove_const_t = __remove_qualifiers(T, const);
>   
>   
>   template
>   using remove_reference_t = __remove_qualifiers(T, &, &&);
>   
>   template
>   using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, 
> &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t

I don't think it's worth adding that parsing complexity for a builtin that we 
expect to only be used in system headers.  Let's just remove `const` and 
`volatile` and leave other qualifiers in place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-06 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

In D116203#3431612 , @rjmccall wrote:

> In D116203#3430332 , @aaron.ballman 
> wrote:
>
>> In D116203#3425512 , @cjdb wrote:
>>
>>> I've noticed that libstdc++ has `using __remove_cv = typename 
>>> remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
>>> `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
>>> Is there a way we can work around this, or should we just rename 
>>> `__remove_cv` and friends to something else?
>>
>> You could work around it by taking note that you're in a libstdc++ system 
>> header and do a special dance, but because these are in the implementation's 
>> namespace, I think it's probably kinder for everyone to just pick a 
>> different name.

I was hoping we could do something similar to `struct __remove_cv` which would 
issue a warning?

>> If you wanted to be especially mean, you could go with `__remove_cvr`, but 
>> I'd suggest `__remove_cv_qualifiers` instead. However, what about `restrict` 
>> qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf
>
> Along with a fair number of other vendor qualifiers, yeah.  I think you have 
> to make a policy decision about whether the intent of `std::remove_cv` is 
> really to just remove CV qualifiers or to produce an unqualified type (at the 
> outermost level).  The former is probably more defensible, even if it makes 
> the transform less useful in the presence of extended qualifiers.

I'm partial to `std::remove_cv` being faithful to its name, unless existing 
implementations do something else already. I don't mind adding support for the 
other stuff, but if there's more than just add/remove `restrict`, we're going 
to have a combinatorial explosion for removes. Is there an alternate way we can 
approach this?
Possibly:

  template
  using remove_const_t = __remove_qualifiers(T, const);
  
  
  template
  using remove_reference_t = __remove_qualifiers(T, &, &&);
  
  template
  using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, &, 
&&); // rcv instead of cvr to prevent a typo with remove_cvref_t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-05 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

In D116203#3430332 , @aaron.ballman 
wrote:

> In D116203#3425512 , @cjdb wrote:
>
>> I've noticed that libstdc++ has `using __remove_cv = typename 
>> remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
>> `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
>> Is there a way we can work around this, or should we just rename 
>> `__remove_cv` and friends to something else?
>
> You could work around it by taking note that you're in a libstdc++ system 
> header and do a special dance, but because these are in the implementation's 
> namespace, I think it's probably kinder for everyone to just pick a different 
> name.
>
> If you wanted to be especially mean, you could go with `__remove_cvr`, but 
> I'd suggest `__remove_cv_qualifiers` instead. However, what about `restrict` 
> qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf

Along with a fair number of other vendor qualifiers, yeah.  I think you have to 
make a policy decision about whether the intent of `std::remove_cv` is really 
to just remove CV qualifiers or to produce an unqualified type (at the 
outermost level).  The former is probably more defensible, even if it makes the 
transform less useful in the presence of extended qualifiers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-05 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D116203#3425512 , @cjdb wrote:

> I've noticed that libstdc++ has `using __remove_cv = typename 
> remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
> `KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
> Is there a way we can work around this, or should we just rename 
> `__remove_cv` and friends to something else?

You could work around it by taking note that you're in a libstdc++ system 
header and do a special dance, but because these are in the implementation's 
namespace, I think it's probably kinder for everyone to just pick a different 
name.

If you wanted to be especially mean, you could go with `__remove_cvr`, but I'd 
suggest `__remove_cv_qualifiers` instead. However, what about `restrict` 
qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-03 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

I've noticed that libstdc++ has `using __remove_cv = typename 
remove_cv::type`, which causes Clang to chuck a wobbly. Changing from 
`KEYWORD` to `TYPE_TRAIT_1` didn't seem to fix anything.
Is there a way we can work around this, or should we just rename `__remove_cv` 
and friends to something else?




Comment at: clang/lib/Sema/SemaType.cpp:9227
 
-DiagnoseUseOfDecl(ED, Loc);
+  QualType Underlying = Context.getIntTypeForBitwidth(
+  Context.getIntWidth(BaseType), IsMakeSigned);

rjmccall wrote:
> cjdb wrote:
> > erichkeane wrote:
> > > Can you add a couple of tests to make sure this works with _BitInt?  Note 
> > > that this + the libc++ fixes get this done: 
> > > https://github.com/llvm/llvm-project/issues/50427
> > Done for `_BitInt`. Is there a corresponding unsigned `_BitInt`? Neither 
> > `_BitUint`, `BitUInt`, nor `_UBitInt` work :(
> I believe `_BitInt` works like `int` et al., so it'd be `unsigned _BitInt`, 
All done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-02 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

In D116203#3423594 , @cjdb wrote:

> In D116203#3277515 , @zoecarver 
> wrote:
>
>> This patch looks awesome, Chris.
>>
>> Does it make sense to have builtins for `add_const`, etc.? Isn't `T const` 
>> already kind of a builtin for this?
>
> Potentially, but I need a core language lawyer to weigh in here. The library 
> wording for `std::add_const::type` is:
>
>> If `T` is a reference, function, or top-level `const`-qualified type, then 
>> type names the same type as `T`, otherwise `T const`.
>
> Although my understanding is that the `T const` in this case is redundant 
> (and thus not applied per dcl.type.cv 
> ), the library wording goes out of its 
> way to say "do nothing".

The language ignores attempts to add most qualifiers to function and reference 
types.  (The only exception is that we allow address space qualifiers on 
function types, but that's extension territory.)  Compilers will warn or error 
about it in various situations if you do it directly, but not if you do it via 
a template or a typedef.  This is [dcl.type.cv]p1 (redundant const), 
[dcl.fct]p7 (function types), and [dcl.ref]p1 (references).

I think the spec is just trying to say that this isn't an end-around the other 
language rules and it doesn't construct const-qualified versions of certain 
types that otherwise the language is at pains to never construct.

Comment at: clang/lib/Sema/SemaType.cpp:9227

-DiagnoseUseOfDecl(ED, Loc);
+  QualType Underlying = Context.getIntTypeForBitwidth(
+  Context.getIntWidth(BaseType), IsMakeSigned);

cjdb wrote:
> erichkeane wrote:
> > Can you add a couple of tests to make sure this works with _BitInt?  Note 
> > that this + the libc++ fixes get this done: 
> > https://github.com/llvm/llvm-project/issues/50427
> Done for `_BitInt`. Is there a corresponding unsigned `_BitInt`? Neither 
> `_BitUint`, `BitUInt`, nor `_UBitInt` work :(
I believe `_BitInt` works like `int` et al., so it'd be `unsigned _BitInt`, 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-04-01 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

In D116203#3255018 , @aaron.ballman 
wrote:

> In D116203#3220165 , @aaron.ballman 
> wrote:
>
>> The summary for the patch explains what's being added, but there's really no 
>> information as to why it's being added. Why do we need these builtins?
>
> Btw, I still think the patch summary should be updated with this information.

Sorry, I completely missed this. Done.

In D116203#3277515 , @zoecarver wrote:

> This patch looks awesome, Chris.
>
> Does it make sense to have builtins for `add_const`, etc.? Isn't `T const` 
> already kind of a builtin for this?

Potentially, but I need a core language lawyer to weigh in here. The library 
wording for `std::add_const::type` is:

> If `T` is a reference, function, or top-level `const`-qualified type, then 
> type names the same type as `T`, otherwise `T const`.

Although my understanding is that the `T const` in this case is redundant (and 
thus not applied per dcl.type.cv ), the 
library wording goes out of its way to say "do nothing".




Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > A function without a prototype is referenceable? (This is more of a 
> > > "should this predicate do anything in C?" kind of question, I suppose.)
> > Does C++ have a notion of non-prototyped functions? I don't think it does? 
> > As such, I'm struggling to model this in a way that makes sense :(
> > 
> > Maybe that's evidence for NAD, maybe it isn't :shrug:
> C++ doesn't have the notion of a function without a prototype (thankfully).
> 
> Should this function assert that it's not called in C mode at all? I don't 
> think asking the question makes sense in C.
I made a change in that this asserts when not in C++ mode.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > This can be reformatted, I believe, but did you look around to see if an 
> > > existing diagnostic would suffice?
> > Do you have any tips on how to narrow my search? I don't really want to 
> > //read// 11K lines of diagnostics.
> I usually search for "typecheck" for type checking diagnostics. One that 
> looks close is:
> ```
> def err_pragma_loop_invalid_argument_type : Error<
>   "invalid argument of type %0; expected an integer type">;
> ```
> that could likely be changed to something along the lines of:
> ```
> def err_invalid_argument_type : Error<
>   "invalid argument of type %0; expected an integer type %select{|other than 
> 'bool'}1">;
> ```
> (I think enum types are always integer types and so they may not need to be 
> called out in the diagnostic.)
I'm not particularly fond of this one because it doesn't call out the builtin's 
user-facing name. I suppose we could do this, but I'm not sure where to draw 
the line:

```
def err_invalid_argument_type : Error<
  "invalid argument %select{|for 'make_%select{signed|unsigned}2'}1, with type 
%0; expected an integer type %select{|other than 'bool'}3">;
```

This means that users who misuse `std::make_[un]signed` will get a nice 
diagnostic instead of one targeting `__make_[un]signed`, which helps them work 
out where they can fix their code.



Comment at: clang/lib/AST/ASTContext.cpp:5604
 /// savings are minimal and these are rare.
+// Update 2021-12-16: it might be worth revisiting this
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

aaron.ballman wrote:
> cjdb wrote:
> > WDYT @aaron.ballman?
> Are these expected to be less rare now due to your patch? FWIW, I'm fine 
> revisiting if we can measure some value, but I think that's good as a 
> separate patch.
I've got no idea, which is why I'm flagging it. Each of these built-in traits 
makes a call, so it's no longer just `__underlying_type`. Sounds like I can 
resolve for now.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
 break;
+  case UnaryTransformType::AddConst:
+Out << "2ac";

aaron.ballman wrote:
> Are these the suggested manglings from the Itanium mangling document, or 
> something you invented yourself?
> 
> Should there be corresponding Microsoft manglings or are those already 
> handled magically?
> 
> Also, test coverage for the manglings?
I copied the mangling from D67052 and then inferred for what's missing over 
there. I'll consult the

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-28 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

The patch's description still only says what is being added, but provides zero 
motivation as to why it does what it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-27 Thread Zoe Carver via Phabricator via cfe-commits

zoecarver added a comment.

This patch looks awesome, Chris.

Does it make sense to have builtins for `add_const`, etc.? Isn't `T const` 
already kind of a builtin for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-19 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

No comments other than the ones others have made, though the 
make_signed/make_unsigned needs to work for _BitInt




Comment at: clang/lib/Sema/SemaType.cpp:9113
+  BaseType.isReferenceable() || BaseType->isVoidType()
+  ? BuildPointerType(BaseType.getNonReferenceType(), Loc, EntityName)
+  : BaseType;

Do we at any point want this builtin to be address-space aware?  



Comment at: clang/lib/Sema/SemaType.cpp:9227
 
-DiagnoseUseOfDecl(ED, Loc);
+  QualType Underlying = Context.getIntTypeForBitwidth(
+  Context.getIntWidth(BaseType), IsMakeSigned);

Can you add a couple of tests to make sure this works with _BitInt?  Note that 
this + the libc++ fixes get this done: 
https://github.com/llvm/llvm-project/issues/50427


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-19 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added reviewers: rsmith, rjmccall, erichkeane.
aaron.ballman added a comment.

Adding a few more reviewers in case there are more opinions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-19 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D116203#3220165 , @aaron.ballman 
wrote:

> The summary for the patch explains what's being added, but there's really no 
> information as to why it's being added. Why do we need these builtins?

Btw, I still think the patch summary should be updated with this information.




Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

cjdb wrote:
> aaron.ballman wrote:
> > A function without a prototype is referenceable? (This is more of a "should 
> > this predicate do anything in C?" kind of question, I suppose.)
> Does C++ have a notion of non-prototyped functions? I don't think it does? As 
> such, I'm struggling to model this in a way that makes sense :(
> 
> Maybe that's evidence for NAD, maybe it isn't :shrug:
C++ doesn't have the notion of a function without a prototype (thankfully).

Should this function assert that it's not called in C mode at all? I don't 
think asking the question makes sense in C.



Comment at: clang/include/clang/AST/Type.h:6482
+  //   cv-qualifiers or a ref-qualifier, or a reference type.
+  assert(getTypePtr() && "QualType must be valid in order to be 
referenceable");
+  const Type  = **this;

Nope, that will still assert. Here's a better approach.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

cjdb wrote:
> aaron.ballman wrote:
> > This can be reformatted, I believe, but did you look around to see if an 
> > existing diagnostic would suffice?
> Do you have any tips on how to narrow my search? I don't really want to 
> //read// 11K lines of diagnostics.
I usually search for "typecheck" for type checking diagnostics. One that looks 
close is:
```
def err_pragma_loop_invalid_argument_type : Error<
  "invalid argument of type %0; expected an integer type">;
```
that could likely be changed to something along the lines of:
```
def err_invalid_argument_type : Error<
  "invalid argument of type %0; expected an integer type %select{|other than 
'bool'}1">;
```
(I think enum types are always integer types and so they may not need to be 
called out in the diagnostic.)



Comment at: clang/lib/AST/ASTContext.cpp:5604
 /// savings are minimal and these are rare.
+// Update 2021-12-16: it might be worth revisiting this
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

cjdb wrote:
> WDYT @aaron.ballman?
Are these expected to be less rare now due to your patch? FWIW, I'm fine 
revisiting if we can measure some value, but I think that's good as a separate 
patch.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
 break;
+  case UnaryTransformType::AddConst:
+Out << "2ac";

Are these the suggested manglings from the Itanium mangling document, or 
something you invented yourself?

Should there be corresponding Microsoft manglings or are those already handled 
magically?

Also, test coverage for the manglings?



Comment at: clang/lib/Sema/DeclSpec.cpp:597
   case DeclSpec::TST_decltype_auto: return "decltype(auto)";
+  // clang-format off
   case DeclSpec::TST_underlyingType: return "__underlying_type";

cjdb wrote:
> aaron.ballman wrote:
> > We don't typically add clang-format markings to the source files. I think 
> > this should be removed (it disables the formatting for the remainder of the 
> > file).
> My intention was always to delete these pre-merge. clang-format has some 
> really strong opinions on this that breaks consistency with the rest of the 
> file, and it was disrupting CI.
> 
> > (it disables the formatting for the remainder of the file).
> 
> Line 615 should prevent this :-)
Ah, that makes sense and is fine by me, thanks for letting me know.



Comment at: clang/lib/Sema/SemaType.cpp:9121
+SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);

cjdb wrote:
> aaron.ballman wrote:
> > Should we care about ObjC pointers (which are a bit special)?
> What's the compat story between ObjC and C++?
Objective-C++ is a thing: 
https://en.wikipedia.org/wiki/Objective-C#Objective-C++

The salient point is that the type system models ObjC pointers as being 
distinct from pointers. e.g., 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L6702


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-18 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added a comment.

Gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-05 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb added inline comments.



Comment at: clang/include/clang/AST/Type.h:6481
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type  = **this;
+  if (Self.isObjectType() || Self.isReferenceType())

aaron.ballman wrote:
> This is unsafe -- the `Type *` can be null if the `QualType` is invalid.
I think this is fixed?



Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

aaron.ballman wrote:
> A function without a prototype is referenceable? (This is more of a "should 
> this predicate do anything in C?" kind of question, I suppose.)
Does C++ have a notion of non-prototyped functions? I don't think it does? As 
such, I'm struggling to model this in a way that makes sense :(

Maybe that's evidence for NAD, maybe it isn't :shrug:



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

aaron.ballman wrote:
> This can be reformatted, I believe, but did you look around to see if an 
> existing diagnostic would suffice?
Do you have any tips on how to narrow my search? I don't really want to 
//read// 11K lines of diagnostics.



Comment at: clang/lib/AST/ASTContext.cpp:5604
 /// savings are minimal and these are rare.
+// Update 2021-12-16: it might be worth revisiting this
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

WDYT @aaron.ballman?



Comment at: clang/lib/Sema/DeclSpec.cpp:597
   case DeclSpec::TST_decltype_auto: return "decltype(auto)";
+  // clang-format off
   case DeclSpec::TST_underlyingType: return "__underlying_type";

aaron.ballman wrote:
> We don't typically add clang-format markings to the source files. I think 
> this should be removed (it disables the formatting for the remainder of the 
> file).
My intention was always to delete these pre-merge. clang-format has some really 
strong opinions on this that breaks consistency with the rest of the file, and 
it was disrupting CI.

> (it disables the formatting for the remainder of the file).

Line 615 should prevent this :-)



Comment at: clang/lib/Sema/SemaType.cpp:1694
 if (Result.isNull()) {
-  Result = Context.IntTy;
+  if (DS.getTypeSpecType() == DeclSpec::TST_underlyingType)
+Result = Context.IntTy;

aaron.ballman wrote:
> The point to this was for error recovery; can we recover enough type 
> information from the non-underlying type cases to recover similarly?
Looks like we can get away with removing the selection.



Comment at: clang/lib/Sema/SemaType.cpp:9121
+SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);

aaron.ballman wrote:
> Should we care about ObjC pointers (which are a bit special)?
What's the compat story between ObjC and C++?



Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(

aaron.ballman wrote:
> What about things like type attributes (are those lost during decay)?
According to https://eel.is/c++draft/tab:meta.trans.other, no.



Comment at: clang/test/SemaCXX/type-traits.cpp:3447
+{ int a[T(__is_same(add_cv_t, const volatile M))]; }
+{ int a[T(__is_same(add_lvalue_reference_t, M))]; }
+{ int a[T(__is_same(add_pointer_t, M))]; }

aaron.ballman wrote:
> Shouldn't this be the same as `M&`?
> 
> Actually, something funky is going on that I've not looked into very far. 
> When I try the test out and use `M&` instead of `M` here: 
> https://godbolt.org/z/rx5bcsfqe, so we should be sure we're instantiating the 
> tests we expect to instantiate.
Appreciate that you're thorough in your reviews! (Seriously: it's easy to start 
glazing over tests like this).

Per https://eel.is/c++draft/meta.trans.ref, `add_lvalue_reference_t` should 
be `T` whenever it's not a referenceable type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2022-01-04 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

The summary for the patch explains what's being added, but there's really no 
information as to why it's being added. Why do we need these builtins?




Comment at: clang/include/clang/AST/Type.h:743
 
+  bool isReferenceable() const;
+

Doc comments would be handy here because this is a pretty recent term of art.



Comment at: clang/include/clang/AST/Type.h:6480
+  //   type that is either an object type, a function type that does not have
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type  = **this;





Comment at: clang/include/clang/AST/Type.h:6481
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type  = **this;
+  if (Self.isObjectType() || Self.isReferenceType())

This is unsafe -- the `Type *` can be null if the `QualType` is invalid.



Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs();
+  return F == nullptr ||
+ (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);

A function without a prototype is referenceable? (This is more of a "should 
this predicate do anything in C?" kind of question, I suppose.)



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool 
integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<

This can be reformatted, I believe, but did you look around to see if an 
existing diagnostic would suffice?



Comment at: clang/lib/Sema/DeclSpec.cpp:597
   case DeclSpec::TST_decltype_auto: return "decltype(auto)";
+  // clang-format off
   case DeclSpec::TST_underlyingType: return "__underlying_type";

We don't typically add clang-format markings to the source files. I think this 
should be removed (it disables the formatting for the remainder of the file).



Comment at: clang/lib/Sema/SemaType.cpp:1265
+TSTToUnaryTransformType(DeclSpec::TST SwitchTST) {
+  switch (SwitchTST) { // clang-format off
+  case TST_add_const:return UnaryTransformType::AddConst;

Same comments here about the clang-format markup.



Comment at: clang/lib/Sema/SemaType.cpp:1694
 if (Result.isNull()) {
-  Result = Context.IntTy;
+  if (DS.getTypeSpecType() == DeclSpec::TST_underlyingType)
+Result = Context.IntTy;

The point to this was for error recovery; can we recover enough type 
information from the non-underlying type cases to recover similarly?



Comment at: clang/lib/Sema/SemaType.cpp:6023
 void VisitUnaryTransformTypeLoc(UnaryTransformTypeLoc TL) {
-  // FIXME: This holds only because we only have one unary transform.
-  assert(DS.getTypeSpecType() == DeclSpec::TST_underlyingType);
+  // Make sure it is a unary transform type
+  assert(DS.getTypeSpecType() >= DeclSpec::TST_underlyingType &&





Comment at: clang/lib/Sema/SemaType.cpp:9121
+SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+return Context.getUnaryTransformType(BaseType, BaseType, UKind);

Should we care about ObjC pointers (which are a bit special)?



Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(

What about things like type attributes (are those lost during decay)?



Comment at: clang/test/SemaCXX/type-traits.cpp:3447
+{ int a[T(__is_same(add_cv_t, const volatile M))]; }
+{ int a[T(__is_same(add_lvalue_reference_t, M))]; }
+{ int a[T(__is_same(add_pointer_t, M))]; }

Shouldn't this be the same as `M&`?

Actually, something funky is going on that I've not looked into very far. When 
I try the test out and use `M&` instead of `M` here: 
https://godbolt.org/z/rx5bcsfqe, so we should be sure we're instantiating the 
tests we expect to instantiate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2021-12-26 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb updated this revision to Diff 396248.
cjdb edited the summary of this revision.
cjdb added a comment.

adds more type traits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Format/FormatToken.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/type-traits.cpp

Index: clang/test/SemaCXX/type-traits.cpp
===
--- clang/test/SemaCXX/type-traits.cpp
+++ clang/test/SemaCXX/type-traits.cpp
@@ -2854,3 +2854,552 @@
 #undef T16384
 #undef T32768
 } // namespace type_trait_expr_numargs_overflow
+
+struct S {};
+void check_add_const() {
+  { int a[T(__is_same(__add_const(void), const void))]; }
+  { int a[T(__is_same(__add_const(int), const int))]; }
+  { int a[T(__is_same(__add_const(const int), const int))]; }
+  { int a[T(__is_same(__add_const(volatile int), const volatile int))]; }
+  { int a[T(__is_same(__add_const(const volatile int), const volatile int))]; }
+  { int a[T(__is_same(__add_const(int*), int* const))]; }
+  { int a[T(__is_same(__add_const(int&), int&))]; }
+  { int a[T(__is_same(__add_const(int&&), int&&))]; }
+  { int a[T(__is_same(__add_const(int()), int()))]; }
+  { int a[T(__is_same(__add_const(int(*)()), int(* const)()))]; }
+  { int a[T(__is_same(__add_const(int(&)()), int(&)()))]; }
+
+  { int a[T(__is_same(__add_const(S), const S))]; }
+  { int a[T(__is_same(__add_const(const S), const S))]; }
+  { int a[T(__is_same(__add_const(volatile S), const volatile S))]; }
+  { int a[T(__is_same(__add_const(const volatile S), const volatile S))]; }
+  { int a[T(__is_same(__add_const(int S::*), int S::* const))]; }
+  { int a[T(__is_same(__add_const(int (S::*)()), int (S::* const)()))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() &), int (S::* const)() &))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() &&), int (S::* const)() &&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const), int (S::* const)() const))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const&), int (S::* const)() const&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const&&), int (S::* const)() const&&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() volatile), int (S::* const)() volatile))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() volatile&), int (S::* const)() volatile&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() volatile&&), int (S::* const)() volatile&&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const volatile), int (S::* const)() const volatile))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const volatile&), int (S::* const)() const volatile&))]; }
+  { int a[T(__is_same(__add_const(int (S::*)() const volatile&&), int (S::* const)() const volatile&&))]; }
+}
+
+void check_remove_const() {
+  { int a[T(__is_same(__remove_const(void), void))]; }
+  { int a[T(__is_same(__remove_const(const void), void))]; }
+  { int a[T(__is_same(__remove_const(int), int))]; }
+  { int a[T(__is_same(__remove_const(const int), int))]; }
+  { int a[T(__is_same(__remove_const(volatile int), volatile int))]; }
+  { int a[T(__is_same(__remove_const(const volatile int), volatile int))]; }
+  { int a[T(__is_same(__remove_const(int*), int*))]; }
+  { int a[T(__is_same(__remove_const(int* const), int*))]; }
+  { int a[T(__is_same(__remove_const(int const* const), int const*))]; }
+  { int a[T(__is_same(__remove_const(int&), int&))]; }
+  { int a[T(__is_same(__remove_const(int const&), int const&))]; }
+  { int a[T(__is_same(__remove_const(int&&), int&&))]; }
+  { int a[T(__is_same(__remove_const(int const&&), int const&&))]; }
+  { int a[T(__is_same(__remove_const(int()), int()))]; }
+  { int a[T(__is_same(__remove_const(int(* const)()), int(*)()))]; }
+  { int a[T(__is_same(__remove_const(int(&)()), int(&)()))]; }
+
+  { int a[T(__is_same(__remove_const(S), S))]; }
+  { int a[T(__is_same(__remove_const(const S),  S))]; }
+  { int a[T(__is_same(__remove_const(volatile S), volatile S))]; }
+  { int a[T(__is_same(__remove_const(const volatile S), volatile S))]; }
+  { int a[T(__is_same(__remove_const(int S::* const), int S::*))]; }
+  { int a[T(__is_same(__remove_const(int (S::* const)()), int (S::*)()))]; }
+  { int a[T(__is_same(__remove_const(int (S::* const)() &), int (S::*)() &))]; }
+  { int a[T(__is_same(__remove_const(int (S::* const)() &&),

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2021-12-24 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb updated this revision to Diff 396178.
cjdb added a comment.

minor fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Format/FormatToken.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/add_cv.cpp
  clang/test/SemaCXX/remove_cvref.cpp

Index: clang/test/SemaCXX/remove_cvref.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/remove_cvref.cpp
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -std=c++11 %s
+
+template 
+struct check_remove_reference {
+  constexpr check_remove_reference() {
+static_assert(__is_same(__remove_reference(T &), T), "");
+static_assert(__is_same(__remove_reference(const T &), const T), "");
+static_assert(__is_same(__remove_reference(volatile T &), volatile T), "");
+static_assert(__is_same(__remove_reference(const volatile T &), const volatile T), "");
+
+static_assert(__is_same(__remove_reference(T &&), T), "");
+static_assert(__is_same(__remove_reference(const T &&), const T), "");
+static_assert(__is_same(__remove_reference(volatile T &&), volatile T), "");
+static_assert(__is_same(__remove_reference(const volatile T &&), const volatile T), "");
+
+static_assert(__is_same(__remove_cvref(T &), T), "");
+static_assert(__is_same(__remove_cvref(const T &), T), "");
+static_assert(__is_same(__remove_cvref(volatile T &), T), "");
+static_assert(__is_same(__remove_cvref(const volatile T &), T), "");
+
+static_assert(__is_same(__remove_cvref(T &&), T), "");
+static_assert(__is_same(__remove_cvref(const T &&), T), "");
+static_assert(__is_same(__remove_cvref(volatile T &&), T), "");
+static_assert(__is_same(__remove_cvref(const volatile T &&), T), "");
+  }
+};
+
+template <> struct check_remove_reference {};
+template  struct check_remove_reference {};
+template  struct check_remove_reference {};
+
+template 
+constexpr bool check_remove_qualifiers() {
+  static_assert(__is_same(__remove_const(const T), T), "");
+  static_assert(__is_same(__remove_const(volatile T), volatile T), "");
+  static_assert(__is_same(__remove_const(const volatile T), volatile T), "");
+
+  static_assert(__is_same(__remove_volatile(const T), const T), "");
+  static_assert(__is_same(__remove_volatile(volatile T), T), "");
+  static_assert(__is_same(__remove_volatile(const volatile T), const T), "");
+
+  static_assert(__is_same(__remove_cv(const T), T), "");
+  static_assert(__is_same(__remove_cv(volatile T), T), "");
+  static_assert(__is_same(__remove_cv(const volatile T), T), "");
+
+  check_remove_reference();
+
+  return true;
+}
+
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+
+struct S {};
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
Index: clang/test/SemaCXX/add_cv.cpp
===
--- /dev/null
+++

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

2021-12-24 Thread Christopher Di Bella via Phabricator via cfe-commits

cjdb updated this revision to Diff 396177.
cjdb retitled this revision from "[clang] adds `__add_const` as a compiler 
built-in" to "[clang] adds unary type transformations as compiler built-ins".
cjdb edited the summary of this revision.
cjdb added a comment.

merges D116205 , D116206 
, D116226 , 
D116242 , D116260 
, D116262  
and D116264 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/Specifiers.h
  clang/include/clang/Basic/TokenKinds.def
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/DeclSpec.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/lib/Format/FormatToken.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/DeclSpec.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaCXX/add_cv.cpp
  clang/test/SemaCXX/remove_cvref.cpp

Index: clang/test/SemaCXX/remove_cvref.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/remove_cvref.cpp
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -std=c++11 %s
+
+template 
+struct check_remove_reference {
+  constexpr check_remove_reference() {
+static_assert(__is_same(__remove_reference(T &), T), "");
+static_assert(__is_same(__remove_reference(const T &), const T), "");
+static_assert(__is_same(__remove_reference(volatile T &), volatile T), "");
+static_assert(__is_same(__remove_reference(const volatile T &), const volatile T), "");
+
+static_assert(__is_same(__remove_reference(T &&), T), "");
+static_assert(__is_same(__remove_reference(const T &&), const T), "");
+static_assert(__is_same(__remove_reference(volatile T &&), volatile T), "");
+static_assert(__is_same(__remove_reference(const volatile T &&), const volatile T), "");
+
+static_assert(__is_same(__remove_cvref(T &), T), "");
+static_assert(__is_same(__remove_cvref(const T &), T), "");
+static_assert(__is_same(__remove_cvref(volatile T &), T), "");
+static_assert(__is_same(__remove_cvref(const volatile T &), T), "");
+
+static_assert(__is_same(__remove_cvref(T &&), T), "");
+static_assert(__is_same(__remove_cvref(const T &&), T), "");
+static_assert(__is_same(__remove_cvref(volatile T &&), T), "");
+static_assert(__is_same(__remove_cvref(const volatile T &&), T), "");
+  }
+};
+
+template <> struct check_remove_reference {};
+template  struct check_remove_reference {};
+template  struct check_remove_reference {};
+
+template 
+constexpr bool check_remove_qualifiers() {
+  static_assert(__is_same(__remove_const(const T), T), "");
+  static_assert(__is_same(__remove_const(volatile T), volatile T), "");
+  static_assert(__is_same(__remove_const(const volatile T), volatile T), "");
+
+  static_assert(__is_same(__remove_volatile(const T), const T), "");
+  static_assert(__is_same(__remove_volatile(volatile T), T), "");
+  static_assert(__is_same(__remove_volatile(const volatile T), const T), "");
+
+  static_assert(__is_same(__remove_cv(const T), T), "");
+  static_assert(__is_same(__remove_cv(volatile T), T), "");
+  static_assert(__is_same(__remove_cv(const volatile T), T), "");
+
+  check_remove_reference();
+
+  return true;
+}
+
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+
+struct S {};
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");
+static_assert(check_remove_qualifiers(), "");

58 matches

Mail list logo