mgehre marked 5 inline comments as done. mgehre added a comment. In D64448#1577779 <https://reviews.llvm.org/D64448#1577779>, @gribozavr wrote:
> For example, libc++ wraps everything in std in an inline namespace. I believed that I had written a test for inline namespaces, but seems that I shouldn't be working in the middle of the night. I will add one. In D64448#1577779 <https://reviews.llvm.org/D64448#1577779>, @gribozavr wrote: > I don't know how various versions of libstdc++ differ. The current implementation passed the (partial) test case https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5, libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0, libstdc++ 8.3.0 and libstdc++ 9.1.0. ================ 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) ---------------- gribozavr wrote: > 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. I copied that approach from https://reviews.llvm.org/D50119. If people do it in production code, then at least the two leading underscores should tell them "I'm not supposed to use this". I considered a test through warnings, but how would I trigger them? Add `-Wlifetime-categories-debug` which emits notes whenever a [[gsl::Owner/Pointer]] is implicitly/explicitly attached to class? ================ Comment at: clang/test/AST/ast-dump-attr.cpp:222 + +class [[gsl::Pointer]] PointerWithoutArgument{}; +// CHECK: CXXRecordDecl{{.*}} class PointerWithoutArgument ---------------- gribozavr wrote: > This test and related code changes could be split off into a separate patch. I was thinking whether it made sense to separate - fixing the AST dump of attributes with optional type parameter when there is not such attribute - introduce and attribute with optional type parameter while AST dumping it is broken so I decided that both are closely related. Otherwise the fix and its test are in separate PRs? ================ Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:86 + +// Test builtin annotation for std types. +namespace std { ---------------- gribozavr wrote: > 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. This is imitating techniques from different libraries - all techniques that this implementation supports. To check if all techniques that this implementation supports are enough for real standard library implementations, I use https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp against them. Various versions of libstdc++ and libc++ passed. I will test MSVC standard library next. If they would use a technique that this implementation does not support yet, I will add support for that and the corresponding test here. I might fix MSVC support (if needed) only in the following PR: ================ Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:103 +template <typename T> +class set_iterator {}; + ---------------- gribozavr wrote: > Is it actually defined like that? There might be a standard library implementation that does it like this. This tests that we will use the `using iterator = set_iterator<T>;` in `class set {` to attach the [[gsl::Pointer]] to the `set_iterator`. ================ Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp:140 + +class thread; +static_assert(!__is_gsl_pointer(thread), ""); ---------------- gribozavr wrote: > 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;") Agreed, I will rename this to an obviously-fictional name. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64448/new/ https://reviews.llvm.org/D64448 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits