tahonermann added inline comments.

================
Comment at: clang/lib/AST/Type.cpp:1962
 
+bool Type::isType(const std::string TypeName) const {
+  QualType Desugar = this->getLocallyUnqualifiedSingleStepDesugaredType();
----------------
MarcusJohnson91 wrote:
> efriedma wrote:
> > MarcusJohnson91 wrote:
> > > aaron.ballman wrote:
> > > > Oh, I see now that this is doing a name comparison against the type -- 
> > > > that's not a good API in general because it's *really* hard to guess at 
> > > > what the type will come out as textually. e.g., `class` and `struct` 
> > > > keywords are interchangeable in C++, C sometimes gets confused with 
> > > > `bool` vs `_Bool`, template arguments sometimes matter, nested name 
> > > > specifiers, etc. Callers of this API will have to guess at these 
> > > > details and the printing of the type may change over time (e.g., C may 
> > > > switch from `_Bool` to `bool` and then code calling `isType("_Bool")` 
> > > > may react poorly to the change).
> > > > 
> > > > I think we need to avoid this sort of API on `Type`.
> > > I see your point, I reverted the behavior back to doing the desugaring in 
> > > just isChar16Type and isChar32Type
> > I'm not convinced we should be looking at sugar even in 
> > isChar16Type/isChar32Type/isAnyCharacterType.  That seems like a great way 
> > to end up with subtle bugs that only manifest when someone uses the wrong 
> > typedef.
> > 
> > Where is the distinction between the value `(uint32_t)1` vs. `(char32_t)1` 
> > relevant for C, anyway?
> char32_t is a typedef, not a builtin type in C.
> 
> the underlying type is uint_least32_t, which is usually another typedef to 
> int.
> 
> in order for char32_t to be accepted in C mode, we have to know that it is a 
> string type and not just some random array, so I'm checking the sugar to see 
> if char32_t appears in the typedef chain.
> 
> 
You can't, in general, count on typedefs being present.  For example, `U"text"` 
won't have `char32_t` present in its type; the `char32_t` typedef definition 
won't even be present if `uchar.h` is not included.  All you can be 
(reasonably) assured of is that UTF-16 and UTF-32 literals will have a type 
that matches the underlying type of the `char16_t` and `char32_t` typedef 
definition (this can be violated for at least some compilers if you try hard, 
but that reflects an invalid configuration).


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

https://reviews.llvm.org/D103426

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

Reply via email to