rsmith 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: + // ---------------- 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. ================ Comment at: lib/CodeGen/CGBuiltin.cpp:1437-1438 + + // if (!Seen.insert(Record).second) + // return false; + for (FieldDecl *F : Record->fields()) { ---------------- Delete this commented-out code. ================ Comment at: lib/CodeGen/CGBuiltin.cpp:1451 + return false; + llvm::DenseSet<const Decl *> Seen; + return TypeRequiresBuiltinLaunderImp(CGM.getContext(), Ty, Seen); ---------------- Would `SmallPtrSet` be a better choice here? ================ 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); ---------------- 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". ================ Comment at: lib/Sema/SemaChecking.cpp:935 return true; + } ---------------- Did you mean to add this blank line? ================ 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 ---------------- 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]], ``` ================ 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 ---------------- This is likewise likely to fail with a release build of clang. ================ 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), ""); ---------------- `&i` doesn't what? 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