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

[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 &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