[1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
diff mbox series

Message ID 20190813133217.GA11146@redhat.com
State New
Headers show
Series
  • [1/2] PR c++/91436 fix C++ dialect for std::make_unique fix-it hint
Related show

Commit Message

Jonathan Wakely Aug. 13, 2019, 1:32 p.m. UTC
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 <jwakely@redhat.com>
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.

Comments

Jason Merrill Aug. 13, 2019, 8:07 p.m. UTC | #1
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
Jonathan Wakely Aug. 14, 2019, 11:02 a.m. UTC | #2
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<int>();
      |               ^~~~~~~~~~~
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<int>();
      |                           ^~~

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?
Jason Merrill Aug. 14, 2019, 2:38 p.m. UTC | #3
On Wed, Aug 14, 2019 at 7:02 AM Jonathan Wakely <jwakely@redhat.com> 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<int>();
>       |               ^~~~~~~~~~~
> 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<int>();
>       |                           ^~~
>
> 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
David Malcolm Aug. 14, 2019, 2:39 p.m. UTC | #4
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<int>();
>       |               ^~~~~~~~~~~
> 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<int>();
>       |                           ^~~
> 
> 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 <memory> 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?
>
Jason Merrill Aug. 14, 2019, 2:42 p.m. UTC | #5
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<int>();
>>        |               ^~~~~~~~~~~
>> 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<int>();
>>        |                           ^~~
>>
>> 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 <memory> 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
Jonathan Wakely Aug. 14, 2019, 3:53 p.m. UTC | #6
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<int>();
>>       |               ^~~~~~~~~~~
>> 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<int>();
>>       |                           ^~~
>>
>> 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 <memory> 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?
David Malcolm Aug. 14, 2019, 5:48 p.m. UTC | #7
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<int>();
> > >       |               ^~~~~~~~~~~
> > > 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<int>();
> > >       |                           ^~~
> > > 
> > > 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 <memory> 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

Patch
diff mbox series

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", "<bitset>", cxx11},
     /* <complex>.  */
     {"complex", "<complex>", cxx98},
-    {"complex_literals", "<complex>", cxx98},
+    {"complex_literals", "<complex>", cxx14},
     /* <condition_variable>. */
     {"condition_variable", "<condition_variable>", cxx11},
     {"condition_variable_any", "<condition_variable>", cxx11},
@@ -5619,7 +5619,7 @@  get_std_name_hint (const char *name)
     {"multimap", "<map>", cxx98},
     /* <memory>.  */
     {"make_shared", "<memory>", cxx11},
-    {"make_unique", "<memory>", cxx11},
+    {"make_unique", "<memory>", cxx14},
     {"shared_ptr", "<memory>", cxx11},
     {"unique_ptr", "<memory>", cxx11},
     {"weak_ptr", "<memory>", 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 <complex>" "" { 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<class T>
-void test_make_unique ()
-{
-  auto p = std::make_unique<T>(); // { dg-error "'make_unique' is not a member of 'std'" }
-  // { dg-message "'#include <memory>'" "" { target *-*-* } .-1 }
-  // { dg-error "expected primary-expression before '>' token" "" { target *-*-* } .-2 }
-  // { dg-error "expected primary-expression before '\\)' token" "" { target *-*-* } .-3 }
-}
-
 std::shared_ptr<int> test_shared_ptr; // { dg-error "'shared_ptr' in namespace 'std' does not name a template type" }
 // { dg-message "'#include <memory>'" "" { target *-*-* } .-1 }