Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On Wed, 2019-08-14 at 16:53 +0100, Jonathan Wakely wrote: > On 14/08/19 10:39 -0400, David Malcolm wrote: > > On Wed, 2019-08-14 at 12:02 +0100, Jonathan Wakely wrote: > > > On 13/08/19 16:07 -0400, Jason Merrill wrote: > > > > On 8/13/19 9:32 AM, Jonathan Wakely wrote: > > > > > * g++.dg/lookup/missing-std-include-6.C: Don't check > > > > > make_unique in > > > > > test that runs for C++11. > > > > > > > > I'm not comfortable removing this test coverage > > > > entirely. Doesn't > > > > it > > > > give a useful diagnostic in C++11 mode as well? > > > > > > It does: > > > > > > mu.cc:3:15: error: 'make_unique' is not a member of 'std' > > > 3 | auto p = std::make_unique(); > > > | ^~~ > > > mu.cc:3:15: note: 'std::make_unique' is only available from C++14 > > > onwards > > > mu.cc:3:27: error: expected primary-expression before 'int' > > > 3 | auto p = std::make_unique(); > > > | ^~~ > > > > > > So we can add it to g++.dg/lookup/missing-std-include-8.C > > > instead, > > > which runs for c++98_only and checks for the "is only available > > > for" > > > cases. Here's a patch doing that. > > > > FWIW this eliminates the testing that when we do have C++14 > > onwards, > > that including is suggested. > > Do we really care? > > Are we testing that *every* entry in the array gives the right answer > for both missing-header and bad-std-option, or are we just testing a > subset of them to be sure the logic works as expected? > > Because if we're testing every entry then: > > 1) we're missing LOTS of tests, and > > 2) we're just as likely to test the wrong thing and not actually > catch >bugs (as was already happening for both make_unique and >complex_literals). > > > Maybe we need a C++14-onwards missing-std-include-* test, and to > > move > > the existing test there? (and to add the new test for before-C++- > > 14) > > We could, but is it worth it? Fair enough. Dave
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On 14/08/19 10:39 -0400, David Malcolm wrote: On Wed, 2019-08-14 at 12:02 +0100, Jonathan Wakely wrote: On 13/08/19 16:07 -0400, Jason Merrill wrote: > On 8/13/19 9:32 AM, Jonathan Wakely wrote: > > * g++.dg/lookup/missing-std-include-6.C: Don't check > > make_unique in > > test that runs for C++11. > > I'm not comfortable removing this test coverage entirely. Doesn't > it > give a useful diagnostic in C++11 mode as well? It does: mu.cc:3:15: error: 'make_unique' is not a member of 'std' 3 | auto p = std::make_unique(); | ^~~ mu.cc:3:15: note: 'std::make_unique' is only available from C++14 onwards mu.cc:3:27: error: expected primary-expression before 'int' 3 | auto p = std::make_unique(); | ^~~ So we can add it to g++.dg/lookup/missing-std-include-8.C instead, which runs for c++98_only and checks for the "is only available for" cases. Here's a patch doing that. FWIW this eliminates the testing that when we do have C++14 onwards, that including is suggested. Do we really care? Are we testing that *every* entry in the array gives the right answer for both missing-header and bad-std-option, or are we just testing a subset of them to be sure the logic works as expected? Because if we're testing every entry then: 1) we're missing LOTS of tests, and 2) we're just as likely to test the wrong thing and not actually catch bugs (as was already happening for both make_unique and complex_literals). Maybe we need a C++14-onwards missing-std-include-* test, and to move the existing test there? (and to add the new test for before-C++-14) We could, but is it worth it?
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On 8/14/19 10:39 AM, David Malcolm wrote: On Wed, 2019-08-14 at 12:02 +0100, Jonathan Wakely wrote: On 13/08/19 16:07 -0400, Jason Merrill wrote: On 8/13/19 9:32 AM, Jonathan Wakely wrote: * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. I'm not comfortable removing this test coverage entirely. Doesn't it give a useful diagnostic in C++11 mode as well? It does: mu.cc:3:15: error: 'make_unique' is not a member of 'std' 3 | auto p = std::make_unique(); | ^~~ mu.cc:3:15: note: 'std::make_unique' is only available from C++14 onwards mu.cc:3:27: error: expected primary-expression before 'int' 3 | auto p = std::make_unique(); | ^~~ So we can add it to g++.dg/lookup/missing-std-include-8.C instead, which runs for c++98_only and checks for the "is only available for" cases. Here's a patch doing that. FWIW this eliminates the testing that when we do have C++14 onwards, that including is suggested. Maybe we need a C++14-onwards missing-std-include-* test, and to move the existing test there? (and to add the new test for before-C++-14) We can also check for different messages in different std modes, i.e. { dg-message "one" "" { target c++11_down } .-1 } { dg-message "two" "" { target c++14 } .-2 } Jason
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On Wed, 2019-08-14 at 12:02 +0100, Jonathan Wakely wrote: > On 13/08/19 16:07 -0400, Jason Merrill wrote: > > On 8/13/19 9:32 AM, Jonathan Wakely wrote: > > > * g++.dg/lookup/missing-std-include-6.C: Don't check > > > make_unique in > > > test that runs for C++11. > > > > I'm not comfortable removing this test coverage entirely. Doesn't > > it > > give a useful diagnostic in C++11 mode as well? > > It does: > > mu.cc:3:15: error: 'make_unique' is not a member of 'std' > 3 | auto p = std::make_unique(); > | ^~~ > mu.cc:3:15: note: 'std::make_unique' is only available from C++14 > onwards > mu.cc:3:27: error: expected primary-expression before 'int' > 3 | auto p = std::make_unique(); > | ^~~ > > So we can add it to g++.dg/lookup/missing-std-include-8.C instead, > which runs for c++98_only and checks for the "is only available for" > cases. Here's a patch doing that. FWIW this eliminates the testing that when we do have C++14 onwards, that including is suggested. Maybe we need a C++14-onwards missing-std-include-* test, and to move the existing test there? (and to add the new test for before-C++-14) > Tested x86_64-linux. > > OK for trunk? > > OK for gcc-9-branch and gcc-8-branch too, since PR c++/91436 affects > those branches? >
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On Wed, Aug 14, 2019 at 7:02 AM Jonathan Wakely wrote: > > On 13/08/19 16:07 -0400, Jason Merrill wrote: > >On 8/13/19 9:32 AM, Jonathan Wakely wrote: > >> * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in > >> test that runs for C++11. > > > >I'm not comfortable removing this test coverage entirely. Doesn't it > >give a useful diagnostic in C++11 mode as well? > > It does: > > mu.cc:3:15: error: 'make_unique' is not a member of 'std' > 3 | auto p = std::make_unique(); > | ^~~ > mu.cc:3:15: note: 'std::make_unique' is only available from C++14 onwards > mu.cc:3:27: error: expected primary-expression before 'int' > 3 | auto p = std::make_unique(); > | ^~~ > > So we can add it to g++.dg/lookup/missing-std-include-8.C instead, > which runs for c++98_only and checks for the "is only available for" > cases. Here's a patch doing that. > > Tested x86_64-linux. > > OK for trunk? > > OK for gcc-9-branch and gcc-8-branch too, since PR c++/91436 affects > those branches? OK. Jason
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On 13/08/19 16:07 -0400, Jason Merrill wrote: On 8/13/19 9:32 AM, Jonathan Wakely wrote: * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. I'm not comfortable removing this test coverage entirely. Doesn't it give a useful diagnostic in C++11 mode as well? It does: mu.cc:3:15: error: 'make_unique' is not a member of 'std' 3 | auto p = std::make_unique(); | ^~~ mu.cc:3:15: note: 'std::make_unique' is only available from C++14 onwards mu.cc:3:27: error: expected primary-expression before 'int' 3 | auto p = std::make_unique(); | ^~~ So we can add it to g++.dg/lookup/missing-std-include-8.C instead, which runs for c++98_only and checks for the "is only available for" cases. Here's a patch doing that. Tested x86_64-linux. OK for trunk? OK for gcc-9-branch and gcc-8-branch too, since PR c++/91436 affects those branches? commit 5ad7b3202e4818f2d6d84e22e7e489b39a65c851 Author: Jonathan Wakely Date: Tue Aug 13 13:25:39 2019 +0100 PR c++/91436 fix C++ dialect for std::make_unique fix-it hint The std::make_unique function wasn't added until C++14, and neither was the std::complex_literals namespace. gcc/cp: PR c++/91436 * name-lookup.c (get_std_name_hint): Fix min_dialect field for complex_literals and make_unique entries. gcc/testsuite: PR c++/91436 * g++.dg/lookup/missing-std-include-5.C: Limit test to C++14 and up. * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. * g++.dg/lookup/missing-std-include-8.C: Check make_unique here. diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index d5e491e9072..16c74287bb1 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -5559,7 +5559,7 @@ get_std_name_hint (const char *name) {"bitset", "", cxx11}, /* . */ {"complex", "", cxx98}, -{"complex_literals", "", cxx98}, +{"complex_literals", "", cxx14}, /* . */ {"condition_variable", "", cxx11}, {"condition_variable_any", "", cxx11}, @@ -5632,7 +5632,7 @@ get_std_name_hint (const char *name) {"allocator", "", cxx98}, {"allocator_traits", "", cxx11}, {"make_shared", "", cxx11}, -{"make_unique", "", cxx11}, +{"make_unique", "", cxx14}, {"shared_ptr", "", cxx11}, {"unique_ptr", "", cxx11}, {"weak_ptr", "", cxx11}, diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C index fe880a6263b..3ec9abd9316 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C @@ -1,2 +1,3 @@ +// { dg-do compile { target c++14 } } using namespace std::complex_literals; // { dg-error "" } // { dg-message "#include " "" { target *-*-* } .-1 } diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C index d9eeb4284e8..a8f27473e6d 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C @@ -11,15 +11,6 @@ void test_make_shared () // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } } -template -void test_make_unique () -{ - auto p = std::make_unique(); // { dg-error "'make_unique' is not a member of 'std'" } - // { dg-message "'#include '" "" { target *-*-* } .-1 } - // { dg-error "expected primary-expression before '>' token" "" { target *-*-* } .-2 } - // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } -} - std::shared_ptr test_shared_ptr; // { dg-error "'shared_ptr' in namespace 'std' does not name a template type" } // { dg-message "'#include '" "" { target *-*-* } .-1 } diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-8.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-8.C index 68b208299f2..73532c82968 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-8.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-8.C @@ -13,6 +13,15 @@ void test_make_shared () // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } } +template +void test_make_unique () +{ + std::make_unique(); // { dg-error "'make_unique' is not a member of 'std'" } + // { dg-message "'std::make_unique' is only available from C\\+\\+14 onwards" "" { target *-*-* } .-1 } + // { dg-error "expected primary-expression before '>' token" "" { target *-*-* } .-2 } + // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } +} + void test_array () { std::array a; // { dg-error "'array' is not a member of 'std'" }
Re: [PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
On 8/13/19 9:32 AM, Jonathan Wakely wrote: * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. I'm not comfortable removing this test coverage entirely. Doesn't it give a useful diagnostic in C++11 mode as well? Jason
[PATCH 1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
The std::make_unique function wasn't added until C++14, and neither was the std::complex_literals namespace. gcc/cp: PR c++/91436 * name-lookup.c (get_std_name_hint): Fix min_dialect field for complex_literals and make_unique entries. gcc/testsuite: PR c++/91436 * g++.dg/lookup/missing-std-include-5.C: Limit test to C++14 and up. * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. Tested x86_64-linux. OK for trunk? commit b58e771fe21ae380200fdec00aa899d4b15b73d5 Author: Jonathan Wakely Date: Tue Aug 13 13:25:39 2019 +0100 PR c++/91436 fix C++ dialect for std::make_unique fix-it hint The std::make_unique function wasn't added until C++14, and neither was the std::complex_literals namespace. gcc/cp: PR c++/91436 * name-lookup.c (get_std_name_hint): Fix min_dialect field for complex_literals and make_unique entries. gcc/testsuite: PR c++/91436 * g++.dg/lookup/missing-std-include-5.C: Limit test to C++14 and up. * g++.dg/lookup/missing-std-include-6.C: Don't check make_unique in test that runs for C++11. diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 9f278220df3..96b2d90540d 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -5557,7 +5557,7 @@ get_std_name_hint (const char *name) {"bitset", "", cxx11}, /* . */ {"complex", "", cxx98}, -{"complex_literals", "", cxx98}, +{"complex_literals", "", cxx14}, /* . */ {"condition_variable", "", cxx11}, {"condition_variable_any", "", cxx11}, @@ -5619,7 +5619,7 @@ get_std_name_hint (const char *name) {"multimap", "", cxx98}, /* . */ {"make_shared", "", cxx11}, -{"make_unique", "", cxx11}, +{"make_unique", "", cxx14}, {"shared_ptr", "", cxx11}, {"unique_ptr", "", cxx11}, {"weak_ptr", "", cxx11}, diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C index fe880a6263b..3ec9abd9316 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-5.C @@ -1,2 +1,3 @@ +// { dg-do compile { target c++14 } } using namespace std::complex_literals; // { dg-error "" } // { dg-message "#include " "" { target *-*-* } .-1 } diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C index d9eeb4284e8..a8f27473e6d 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-6.C @@ -11,15 +11,6 @@ void test_make_shared () // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } } -template -void test_make_unique () -{ - auto p = std::make_unique(); // { dg-error "'make_unique' is not a member of 'std'" } - // { dg-message "'#include '" "" { target *-*-* } .-1 } - // { dg-error "expected primary-expression before '>' token" "" { target *-*-* } .-2 } - // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 } -} - std::shared_ptr test_shared_ptr; // { dg-error "'shared_ptr' in namespace 'std' does not name a template type" } // { dg-message "'#include '" "" { target *-*-* } .-1 }