nickdesaulniers added inline comments.
================
Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1131-1132
+ case CK_NullToPointer: {
+ if (llvm::Constant *C = Visit(subExpr, destType))
+ if (C->isNullValue())
+ return CGM.EmitNullConstant(destType);
----------------
efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > FWIW, I would have thought these might be unnecessary, but we do trip 2
> > > > tests without these guards IIRC.
> > > The fact that the Visit can fail doesn't surprise me; you can write
> > > arbitrary expressions of type nullptr_t (for example, `nullptr_t f();
> > > void* g = f();`. I can't think of any reason you'd need to check
> > > C->isNullValue(), though; do you have a testcase for that?
> > Failed Tests (2):
> > Clang :: CodeGenCXX/const-init-cxx11.cpp
> > Clang :: CodeGenCXX/cxx11-thread-local-instantiated.cpp
> >
> > Looking at the first:
> >
> > ```
> > decltype(nullptr) null();
> > int *p = null();
> > ```
> > and the corresponding AST:
> > ```
> > |-FunctionDecl 0x5650ebbb44a8 <x.cpp:2:3, col:26> col:21 used null
> > 'decltype(nullptr) ()'
> > `-VarDecl 0x5650ebbb45e0 <line:3:3, col:17> col:8 p 'int *' cinit
> > `-ImplicitCastExpr 0x5650ebbb4740 <col:12, col:17> 'int *' <NullToPointer>
> > `-CallExpr 0x5650ebbb4720 <col:12, col:17>
> > 'decltype(nullptr)':'std::nullptr_t'
> > `-ImplicitCastExpr 0x5650ebbb4708 <col:12> 'decltype(nullptr) (*)()'
> > <FunctionToPointerDecay>
> > `-DeclRefExpr 0x5650ebbb4690 <col:12> 'decltype(nullptr) ()' lvalue
> > Function 0x5650ebbb44a8 'null' 'decltype(nullptr) ()'
> > ```
> >
> >
> > we change the existing test case from having a `__cxx_global_var_init`
> > routine, i.e.:
> > ```
> > @p = dso_local global ptr null, align 8
> > @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr,
> > ptr } { i32 65535, ptr @_GLOBAL__sub_I_x.cpp, ptr null }]
> >
> > ; Function Attrs: noinline uwtable
> > define internal void @__cxx_global_var_init() #0 section ".text.startup" {
> > entry:
> > %call = call ptr @_Z4nullv()
> > store ptr null, ptr @p, align 8
> > ret void
> > }
> >
> > declare ptr @_Z4nullv() #1
> >
> > ; Function Attrs: noinline uwtable
> > define internal void @_GLOBAL__sub_I_x.cpp() #0 section ".text.startup" {
> > entry:
> > call void @__cxx_global_var_init()
> > ret void
> > }
> > ```
> > to simply:
> > ```
> > @p = dso_local global ptr null, align 8
> > ```
> > So it seems nice to skip having the static constructors, but then
> > `@_Z4nullv` is never invoked. That seems problematic? (I expect the fast
> > path to fail once it recurses down to the DeclRefExpr, but that requires
> > the subexpr of the ImplicitCastExpr be visited).
> >
> > ---
> >
> > So I think the code as written is good to go. WDYT?
> I think you need to check that Visit() succeeds (returns a non-null
> Constant*), but like I said before, I can't see why you need to check
> `C->isNullValue()`
oh, sorry, my reading comprehension skills are sliding off a cliff.
Yeah, that's not necessary for test cases in tree. Removed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156175/new/
https://reviews.llvm.org/D156175
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
