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 &OS) {
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
