EricWF marked 22 inline comments as done. EricWF added inline comments.
================ Comment at: lib/CodeGen/CGBuiltin.cpp:1425-1426 + + // FIXME: We either have an incomplete class type, or we have a class template + // whose instantiation has not been forced. Example: + // ---------------- rsmith wrote: > I think it's a bug that `launder` doesn't require `T` to be a complete type. > Can you file an LWG issue? > > We should also decide whether we want to proactively fix this issue (require > the type to be complete from the `Sema` checking of the builtin and assert > that it's defined here) or not. Apparently I misread the specification. The blanket wording in [[res.on.functions](http://eel.is/c++draft/res.on.functions#2.5)] already prohibits this. Though it's worth noting that GCC doesn't enforce this requirement (nor the one about function pointers). ================ Comment at: lib/CodeGen/CGBuiltin.cpp:1451 + return false; + llvm::DenseSet<const Decl *> Seen; + return TypeRequiresBuiltinLaunderImp(CGM.getContext(), Ty, Seen); ---------------- rsmith wrote: > Would `SmallPtrSet` be a better choice here? I have no idea. I'm ignorant to the internals of both containers. Since you asked the question I'll assume it is and make the change. If you would like, I can do further investigation upon request. ================ Comment at: lib/Sema/SemaChecking.cpp:885-887 + // Don't perform LValue conversions since they may strip things like the + // restrict qualifier + ExprResult Arg = S.DefaultFunctionArrayConversion(OrigArg); ---------------- rsmith wrote: > Instead of performing some of the conversions here and some of them as part > of initialization, I think it'd be more obvious to compute the builtin's > parameter type here (which is the type of the argument if it's not of array > [or function] type, and the decayed type of the argument otherwise), and do > the decay and lvalue-to-rvalue conversion as part of the parameter > initialization below. > > The current code arrangement (and especially this comment) leaves a reader > thinking "but you *need* an lvalue-to-rvalue conversion if the argument is an > lvalue". Ack. I think I've implemented what you requested. Could you please verify? ================ Comment at: lib/Sema/SemaChecking.cpp:935 return true; + } ---------------- rsmith wrote: > Did you mean to add this blank line? Oh Richard, you're too kind. Of course I didn't mean to do that. ================ Comment at: test/CodeGen/builtins.c:404-409 + // CHECK: entry + // CHECK-NEXT: %p.addr = alloca i32* + // CHECK-NEXT: %d = alloca i32* + // CHECK-NEXT: store i32* %p, i32** %p.addr, align 8 + // CHECK-NEXT: [[TMP:%.*]] = load i32*, i32** %p.addr + // CHECK-NEXT: store i32* [[TMP]], i32** %d ---------------- rsmith wrote: > This test is not robust against minor IR differences such as variable or > basic block names changing (some of these change in a release build), and is > testing things that are not related to this builtin (eg, that we produce an > alloca for a function parameter and its relative order to an alloca for a > local variable). > > I would remove everything here other than the load and the store, and add an > explicit check that we don't generate a launder call: > > ``` > // CHECK: [[TMP:%.*]] = load i32*, > // CHECK-NOT: @llvm.launder > // CHECK: store i32* [[TMP]], > ``` If you're referring to the behavior of discarding value names, Clang CC1 only does that if `-discard-value-names` is explicitly passed, regardless of how the compiler is built, and so the names should be stable. (The Clang driver still conditionally passes `-discard-value-names` in release builds). Are there other IR changes you have in mind? None the less, I'll simply this test as requested. ================ Comment at: test/CodeGenCXX/builtin-launder.cpp:16 +extern "C" void test_builtin_launder_virtual_fn(TestVirtualFn *p) { + // CHECK: entry + // CHECK-NEXT: %p.addr = alloca [[TYPE:%.*]], align 8 ---------------- rsmith wrote: > This is likewise likely to fail with a release build of clang. I just verified that they pass in both release and debug builds. I previously changed Clang and LLVM to make the label names consistent between the two configurations for the purposes of testing. I'll still attempt to clean unnecessary checks up. ================ Comment at: test/SemaCXX/builtins.cpp:120 + constexpr int i = 42; + // FIXME: Should this work? Since `&i` doesn't. + static_assert(test_in_constexpr(i), ""); ---------------- rsmith wrote: > `&i` doesn't what? That's a great question. Not sure what I was up to when I wrote that. I tried a couple of things I though I might have been thinking, but they all passed. Removing the comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D40218/new/ https://reviews.llvm.org/D40218 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
