[PATCH] D131423: [clang] fix frontend crash when evaluating type trait

[YingChi Long via Phabricator via cfe-commits]

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