Hi Richard- I finally got a chance to take a look at this patch and run it through the nullptr_t tests that we have, and it seems to solve the problem I was attempting. It would be my preference to commit this.
I currently have my existing tests (from the previous patch) and am happy with the messages. What work to you believe needs to happen in order to get this committed? I note that you didn’t have tests in it and I’m not sure what you were attempting to test initially, but if you can tell me what you were attempting to solve, I could perhaps come up with a handful of tests and commit this. Thanks! -Erich From: Keane, Erich Sent: Friday, December 14, 2018 2:57 PM To: Richard Smith <rich...@metafoo.co.uk> Cc: cfe-commits <cfe-commits@lists.llvm.org> Subject: RE: r349201 - Add extension to always default-initialize nullptr_t. Thanks! I’ll take a look in a bit! -Erich From: Richard Smith [mailto:rich...@metafoo.co.uk] Sent: Friday, December 14, 2018 2:55 PM To: Keane, Erich <erich.ke...@intel.com<mailto:erich.ke...@intel.com>> Cc: cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Subject: Re: r349201 - Add extension to always default-initialize nullptr_t. Attached. With the functional part of your change reverted and this applied, the modified tests still pass except error: 'note' diagnostics expected but not seen: File /usr/local/google/home/richardsmith/llvm-git-1/src/tools/clang/test/Analysis/nullptr.cpp Line 128: 'p' initialized to a null pointer value error: 'note' diagnostics seen but not expected: File /usr/local/google/home/richardsmith/llvm-git-1/src/tools/clang/test/Analysis/nullptr.cpp Line 128: 'p' declared without an initial value ... which seems accurate (but perhaps not useful). On Fri, 14 Dec 2018 at 14:47, Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> wrote: I have a patch I put together a while back as an attempt to fix a different bug, that creates a new CastKind for this operation (didn't work out there, but it might do so here). I'll dig it out and mail it to you in case it's a useful starting point. On Fri, 14 Dec 2018 at 14:42, Keane, Erich via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote: Alright, no problem. I’ll push a revert in a few minutes and give it another try. Perhaps I can suppress the creation of a CK_LValueToRValue in the case where it’s a read from nullptr_t From: Richard Smith [mailto:rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>] Sent: Friday, December 14, 2018 2:41 PM To: Keane, Erich <erich.ke...@intel.com<mailto:erich.ke...@intel.com>> Cc: cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Subject: Re: r349201 - Add extension to always default-initialize nullptr_t. Sorry, I was late with my review comment. I think this is the wrong way to approach this problem. This does not "fix all situations where nullptr_t would seem uninitialized", and it makes our AST representation lose source fidelity. On Fri, 14 Dec 2018 at 14:25, Erich Keane via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote: Author: erichkeane Date: Fri Dec 14 14:22:29 2018 New Revision: 349201 URL: http://llvm.org/viewvc/llvm-project?rev=349201&view=rev Log: Add extension to always default-initialize nullptr_t. Core issue 1013 suggests that having an uninitialied std::nullptr_t be UB is a bit foolish, since there is only a single valid value. This DR reports that DR616 fixes it, which does so by making lvalue-to-rvalue conversions from nullptr_t be equal to nullptr. However, just implementing that results in warnings/etc in many places. In order to fix all situations where nullptr_t would seem uninitialized, this patch instead (as an otherwise transparent extension) default initializes uninitialized VarDecls of nullptr_t. Differential Revision: https://reviews.llvm.org/D53713 Change-Id: I84d72a9290054fa55341e8cbdac43c8e7f25b885 Added: cfe/trunk/test/SemaCXX/nullptr_t-init.cpp (with props) Modified: cfe/trunk/lib/Sema/SemaInit.cpp cfe/trunk/test/Analysis/nullptr.cpp cfe/trunk/test/SemaCXX/ast-print-crash.cpp Modified: cfe/trunk/lib/Sema/SemaInit.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=349201&r1=349200&r2=349201&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaInit.cpp (original) +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Dec 14 14:22:29 2018 @@ -4881,6 +4881,13 @@ static void TryDefaultInitialization(Sem return; } + // As an extension, and to fix Core issue 1013, zero initialize nullptr_t. + // Since there is only 1 valid value of nullptr_t, we can just use that. + if (DestType->isNullPtrType()) { + Sequence.AddZeroInitializationStep(Entity.getType()); + return; + } + // - otherwise, no initialization is performed. // If a program calls for the default initialization of an object of Modified: cfe/trunk/test/Analysis/nullptr.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/nullptr.cpp?rev=349201&r1=349200&r2=349201&view=diff ============================================================================== --- cfe/trunk/test/Analysis/nullptr.cpp (original) +++ cfe/trunk/test/Analysis/nullptr.cpp Fri Dec 14 14:22:29 2018 @@ -125,21 +125,16 @@ struct Type { }; void shouldNotCrash() { - decltype(nullptr) p; // expected-note{{'p' declared without an initial value}} - if (getSymbol()) // expected-note {{Assuming the condition is false}} - // expected-note@-1{{Taking false branch}} - // expected-note@-2{{Assuming the condition is false}} - // expected-note@-3{{Taking false branch}} - // expected-note@-4{{Assuming the condition is true}} - // expected-note@-5{{Taking true branch}} - invokeF(p); // expected-warning{{1st function call argument is an uninitialized value}} - // expected-note@-1{{1st function call argument is an uninitialized value}} + decltype(nullptr) p; // expected-note{{'p' initialized to a null pointer value}} if (getSymbol()) // expected-note {{Assuming the condition is false}} // expected-note@-1{{Taking false branch}} // expected-note@-2{{Assuming the condition is true}} // expected-note@-3{{Taking true branch}} - invokeF(nullptr); // expected-note {{Calling 'invokeF'}} - // expected-note@-1{{Passing null pointer value via 1st parameter 'x'}} + invokeF(p); // expected-note{{Passing null pointer value via 1st parameter 'x'}} + // expected-note@-1{{Calling 'invokeF'}} + if (getSymbol()) // expected-note {{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + invokeF(nullptr); if (getSymbol()) { // expected-note {{Assuming the condition is true}} // expected-note@-1{{Taking true branch}} X *xx = Type().x; // expected-note {{Null pointer value stored to field 'x'}} Modified: cfe/trunk/test/SemaCXX/ast-print-crash.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/ast-print-crash.cpp?rev=349201&r1=349200&r2=349201&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/ast-print-crash.cpp (original) +++ cfe/trunk/test/SemaCXX/ast-print-crash.cpp Fri Dec 14 14:22:29 2018 @@ -7,6 +7,6 @@ // CHECK: struct { // CHECK-NEXT: } dont_crash_on_syntax_error; -// CHECK-NEXT: decltype(nullptr) p; +// CHECK-NEXT: decltype(nullptr) p(/*implicit*/(decltype(nullptr))0); struct { } dont_crash_on_syntax_error /* missing ; */ decltype(nullptr) p; Added: cfe/trunk/test/SemaCXX/nullptr_t-init.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/nullptr_t-init.cpp?rev=349201&view=auto ============================================================================== --- cfe/trunk/test/SemaCXX/nullptr_t-init.cpp (added) +++ cfe/trunk/test/SemaCXX/nullptr_t-init.cpp Fri Dec 14 14:22:29 2018 @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -ffreestanding -Wuninitialized %s +// expected-no-diagnostics +typedef decltype(nullptr) nullptr_t; + +// Ensure no 'uninitialized when used here' warnings (Wuninitialized), for +// nullptr_t always-initialized extension. +nullptr_t default_init() { + nullptr_t a; + return a; +} Propchange: cfe/trunk/test/SemaCXX/nullptr_t-init.cpp ------------------------------------------------------------------------------ svn:eol-style = native Propchange: cfe/trunk/test/SemaCXX/nullptr_t-init.cpp ------------------------------------------------------------------------------ svn:keywords = "Author Date Id Rev URL" Propchange: cfe/trunk/test/SemaCXX/nullptr_t-init.cpp ------------------------------------------------------------------------------ svn:mime-type = text/plain _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
