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
