[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> Those are already there in clang/test/SemaCXX/attr-gsl-owner-pointer.cpp.

I see. Sorry, but that seems insufficient to me -- different libraries use different patterns. For example, libc++ wraps everything in std in an inline namespace. I don't know how various versions of libstdc++ differ.



================
Comment at: clang/include/clang/Basic/TokenKinds.def:486
+TYPE_TRAIT_1(__is_gsl_owner, IsGslOwner, KEYCXX)
+TYPE_TRAIT_1(__is_gsl_pointer, IsGslPointer, KEYCXX)
 KEYWORD(__underlying_type           , KEYCXX)
----------------
I'd suggest to split type trait implementations into a separate patch.

Are these type traits being exposed only for testing? I'm not sure it is a good idea to do that -- people will end up using them in production code -- is that a concern? If so, maybe it would be better to test through warnings.


================
Comment at: clang/test/AST/ast-dump-attr.cpp:222
+
+class [[gsl::Pointer]] PointerWithoutArgument{};
+// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument
----------------
This test and related code changes could be split off into a separate patch.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86
+
+// Test builtin annotation for std types.
+namespace std {
----------------
I feel like these tests would be better off in a separate file.

It would be also good to explain which exact library we are trying to imitate in this test. Different libraries use different coding patterns.


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103
+template <typename T>
+class set_iterator {};
+
----------------
Is it actually defined like that?


================
Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:140
+
+class thread;
+static_assert(!__is_gsl_pointer(thread), "");
----------------
Unclear what this test is testing.

If there's something special about thread (e.g., it looks very much like an owner or a pointer, and a buggy implementation can easily declare thread to be an owner or a pointer), please explain that in a comment.

If you're testing that some random name is not being picked up by inference rules, I'd suggest to use an obviously-fictional name ("class type_unknown_to_compiler;")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448