[PATCH] D67588: Add builtin trait for add/remove cv (and similar)

Eric Fiselier via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 10 13:39:35 PST 2020


EricWF added a comment.

LGTM other than the inline comments.
I'll take a second pass at this once they're addressed.

Let's land the patch this week!



================
Comment at: clang/include/clang/Sema/DeclSpec.h:419
   static bool isTypeRep(TST T) {
     return (T == TST_typename || T == TST_typeofType ||
             T == TST_underlyingType || T == TST_atomic||
----------------
Is the block of values for these identifiers contiguous? Can we work towards writing this as `min <= x <= max`?


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1094
     return DeclSpec::TST_removeReferenceType;
+  case tok::kw___remove_cv:
+    return DeclSpec::TST_removeCV;
----------------
It would be really cool if we could simplify these conversions to `static_cast<DeclSpec::TST>(Tok.getKind() - base);`

That said, it's probably easier to land this patch as is.


================
Comment at: clang/lib/Sema/SemaType.cpp:8475
+  }
+  case UnaryTransformType::AddCV: {
+    QualType Underlying = BaseType.withConst().withVolatile();
----------------
I don't know that we need `add_foo`, because if I want to optimize my type-trait, I'll just write `T const`.

Adding code to clang has a maintenance cost; and that's non-trivial. The `remove_foo` traits add a lot of value, and that justifies the cost.



================
Comment at: clang/test/SemaCXX/add_cv.cpp:43
+  static const bool value =
+    __is_same(typename remove_const<T>::type, T) &&
+    __is_same(typename remove_const<const volatile T>::type, volatile T) &&
----------------
Clang tests normally test:

(A) The diagnostics that emit when people seriously misuse.
(B) Just some failures.


================
Comment at: libcxx/test/libcxx/utilities/meta/stress_tests/stress_test_add_cv.sh.cpp:2
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
I think we probably want to keep the libc++ changes in a separate patch.


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

https://reviews.llvm.org/D67588





More information about the cfe-commits mailing list