inclyc added inline comments.
================
Comment at: clang/lib/Basic/TypeTraits.cpp:64
+#define TYPE_TRAIT_N(Spelling, Name, Key) 0,
+#include "clang/Basic/TokenKinds.def"
+};
----------------
shafik wrote:
> @aaron.ballman do we really have to include this three times? We are defining
> different macros so shouldn't we be able to include is just once?
>
> I see we do this in several other places but a few we don't.
I've tried
```
#define TYPE_TRAIT(N, I, K) N,
#include "clang/Basic/TokenKinds.def"
```
But using enum `TypeTrait` as array index seems to have incorrect mapping.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:3789
-
- if (!Arity && Args.empty()) {
- Diag(EndLoc, diag::err_type_trait_arity)
----------------
shafik wrote:
> Why doesn't this catch our case but moving the check into `evaluateTypeTrait`
> does?
In this case:
```
template<class... Ts> bool b = __is_constructible(Ts...); // Parser: 1 argument
`Ts`, no problem
bool x = b<>; // After template template instantiation tree transformation: 0
argument, but missing checks!
```
`Ts...` is considered as 1 argument in Parsing, so passed this check.
> Why doesn't this catch our case but moving the check into `evaluateTypeTrait`
> does?
This patch moved the check to `Sema::BuildTypeTrait`, this function will be
invoked by tree transformation procedure.
> invoked by tree transformation procedure
In fact: `clang::TreeTransform<(anonymous
namespace)::TemplateInstantiator>::TransformTypeTraitExpr(clang::TypeTraitExpr*)
SemaTemplateInstantiate.cpp`).
I think it is not a good idea to move the check into `evaluateTypeTrait`,
because this function might be designed to return the final evaluation result,
we should perform the check _before_ this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131423/new/
https://reviews.llvm.org/D131423
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
