Re: [patch] delete namespace __gnu_test from C++ testsuite

Re: [patch] delete namespace __gnu_test from C++ testsuite

[Michael Elizabeth Chastain]

I would change this code:

  # simple object, enum
  gdb_test "print test1.value" "\\$\[0-9\]* = egyptian" "simple object, enum"

To:

  send_gdb "test1.value\n"
  gdb_expect {
    -re "\\$\[0-9\]+ = __gnu_test::egyptian\r\n$gdb_prompt $" {
      pass "simple object, enum
    }
    -re "\\$\[0-9\]+ = egyptain\r\n$gdb_prompt $" {
      fail "simple object, enum (gdb/895)"
    }
    -re ".*$gdb_prompt $" {
      fail "simple object, enum"
    }
    timeout {
      fail "simple object, enum (timeout)"
    }

If the FAIL happens with a PR number, then anybody who is reviewing the
results can look at the PR and notice that it says "this was broken in
5.3 as well".  (Also when I do this kind of review, I just look in the
gdb.log files and notice that the gdb output was the same, just the
result changed).

If the FAIL happens without a PR number, that is a genuinely new bug
in gdb (or gcc), and deserves to be treated the same as any other
regression.

In the "= egyptian" case, I would call kfail or setup_kfail as soon as
we start using KFAIL's.  KFAIL is moving way up my priority list.

If the "= egyptian" case is due to bad output from gcc, then I would
add a big comment and then call setup_xfail (or call xfail outright).
I would also want to have a gcc bug # in hand and refer to it in
the test script source.

Michael C

Re: [patch] delete namespace __gnu_test from C++ testsuite

[David Carlton]

On Wed, 18 Dec 2002 13:00:38 -0600, Michael Elizabeth Chastain <mec@shout.net> said:

> I would change this code:
>   # simple object, enum
>   gdb_test "print test1.value" "\\$\[0-9\]* = egyptian" "simple object, enum"

> To:

>   send_gdb "test1.value\n"
>   gdb_expect {
>     -re "\\$\[0-9\]+ = __gnu_test::egyptian\r\n$gdb_prompt $" {
>       pass "simple object, enum
>     }
>     -re "\\$\[0-9\]+ = egyptain\r\n$gdb_prompt $" {
>       fail "simple object, enum (gdb/895)"
>     }
>     -re ".*$gdb_prompt $" {
>       fail "simple object, enum"
>     }
>     timeout {
>       fail "simple object, enum (timeout)"
>     }

I'll add something like this to the testsuite soon.  (Hopefully over
the next few days; definitely before Winter quarter starts.)

> In the "= egyptian" case, I would call kfail or setup_kfail as soon
> as we start using KFAIL's.  KFAIL is moving way up my priority list.

Yup.  I think I'll KFAIL something today; I'm just looking for the
right bug to start with.  One possibility is gdb.c++/annota2.exp's
annotate-quit, corresponding to PR 544.

> If the "= egyptian" case is due to bad output from gcc, then I would
> add a big comment and then call setup_xfail (or call xfail outright).
> I would also want to have a gcc bug # in hand and refer to it in
> the test script source.

We can't get it right without GCC's help.  But currently we wouldn't
get it right even if GCC were helping, so I think that KFAIL is
appropriate for now.

Getting this completely correct is going to be a bit of a delicate
issue, actually: eventually, we'll need to be able to conditially
XFAIL tests based on the compiler version number, if we really want to
be scrupulously correct in this regard.

David Carlton
carlton@math.stanford.edu

[patch] delete namespace __gnu_test from C++ testsuite

[David Carlton]

I've committed the following patches to delete the namespace
__gnu_test from the C++ testsuite files in which it occurs: it's
pointless, illegally-named, and turns those files into tests of
namespace support as well as tests of whatever they're trying to test.
(The patches look longer than they really are: most of them are just
whitespace changes.)  I also updated try_catch to use marker comments
instead of hardwired line numbers, as I'd earlier done with m-data and
m-static.

David Carlton
carlton@math.stanford.edu

2002-12-17  David Carlton  <carlton@math.stanford.edu>

	* gdb.c++/try_catch.cc: Add marker comments.
	* gdb.c++/try_catch.exp: Get line numbers from marker comments.
	* gdb.c++/m-data.cc: Delete namespace __gnu_test.
	* gdb.c++/m-static.cc: Ditto.
	* gdb.c++/m-static1.cc: Ditto.
	* gdb.c++/try_catch.cc: Ditto.

Index: m-data.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/m-data.cc,v
retrieving revision 1.2
diff -u -p -r1.2 m-data.cc
--- m-data.cc	11 Dec 2002 18:45:29 -0000	1.2
+++ m-data.cc	17 Dec 2002 23:20:25 -0000
@@ -1,47 +1,44 @@
 // 2002-05-13
 
-namespace __gnu_test
+enum region { oriental, egyptian, greek, etruscan, roman };
+
+// Test one.
+class gnu_obj_1
 {
-  enum 	region { oriental, egyptian, greek, etruscan, roman };
+protected:
+  typedef region antiquities;
+  const bool 		test;
+  const int 		key1;
+  long       		key2;
+
+  antiquities 	value;
+
+public:
+  gnu_obj_1(antiquities a, long l): test(true), key1(5), key2(l), value(a) {}
+};
 
-  // Test one.
-  class gnu_obj_1
-  {
-  protected:
-    typedef region antiquities;
-    const bool 		test;
-    const int 		key1;
-    long       		key2;
-
-    antiquities 	value;
-
-  public:
-    gnu_obj_1(antiquities a, long l): test(true), key1(5), key2(l), value(a) {}
-  };
-
-  // Test two.
-  template<typename T>
-    class gnu_obj_2: public virtual gnu_obj_1
-    {
-    protected:
-      antiquities	value_derived;
+// Test two.
+template<typename T>
+class gnu_obj_2: public virtual gnu_obj_1
+{
+protected:
+  antiquities	value_derived;
       
-    public:
-      gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7), value_derived(b) { }
-    }; 
-
-  // Test three.
-  template<typename T>
-    class gnu_obj_3
-    {
-    protected:
-      typedef region antiquities;
-      gnu_obj_2<int>   	data;
+public:
+  gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7), value_derived(b) { }
+}; 
+
+// Test three.
+template<typename T>
+class gnu_obj_3
+{
+protected:
+  typedef region antiquities;
+  gnu_obj_2<int>   	data;
       
-    public:
-      gnu_obj_3(antiquities b): data(etruscan) { }
-    }; 
-}
+public:
+  gnu_obj_3(antiquities b): data(etruscan) { }
+}; 
 
 int shadow = 0;
 
@@ -56,7 +53,6 @@ private:
 
 int main()
 {
-  using namespace __gnu_test;
   gnu_obj_1		test1(egyptian, 4589);
   gnu_obj_2<long>	test2(roman);
   gnu_obj_3<long>	test3(greek);
Index: m-static.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/m-static.cc,v
retrieving revision 1.2
diff -u -p -r1.2 m-static.cc
--- m-static.cc	18 Sep 2002 18:48:43 -0000	1.2
+++ m-static.cc	17 Dec 2002 23:20:32 -0000
@@ -1,73 +1,68 @@
 // 2002-05-13
 
-namespace __gnu_test
-{
-  enum 	region { oriental, egyptian, greek, etruscan, roman };
+enum region { oriental, egyptian, greek, etruscan, roman };
 
-  // Test one.
-  class gnu_obj_1
-  {
-  protected:
-    typedef region antiquities;
-    static const bool 	test = true;
-    static const int 	key1 = 5;
-    static long       	key2;
-
-    static antiquities 	value;
-
-  public:
-    gnu_obj_1(antiquities a, long l) {}
-  };
-
-  const bool gnu_obj_1::test;
-  const int gnu_obj_1::key1;
-  long gnu_obj_1::key2 = 77;
-  gnu_obj_1::antiquities gnu_obj_1::value = oriental;
-
-
-  // Test two.
-  template<typename T>
-    class gnu_obj_2: public virtual gnu_obj_1
-    {
-    public:
-      static antiquities	value_derived;
+// Test one.
+class gnu_obj_1
+{
+protected:
+  typedef region antiquities;
+  static const bool 	test = true;
+  static const int 	key1 = 5;
+  static long       	key2;
+
+  static antiquities 	value;
+
+public:
+  gnu_obj_1(antiquities a, long l) {}
+};
+
+const bool gnu_obj_1::test;
+const int gnu_obj_1::key1;
+long gnu_obj_1::key2 = 77;
+gnu_obj_1::antiquities gnu_obj_1::value = oriental;
+
+
+// Test two.
+template<typename T>
+class gnu_obj_2: public virtual gnu_obj_1
+{
+public:
+  static antiquities	value_derived;
       
-    public:
-      gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7) { }
-    }; 
-
-  template<typename T>
-    typename gnu_obj_2<T>::antiquities gnu_obj_2<T>::value_derived = etruscan;
-
-  // Test three.
-  template<typename T>
-    class gnu_obj_3
-    {
-    public:
-      typedef region antiquities;
-      static gnu_obj_2<int>   	data;
+public:
+  gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7) { }
+}; 
+
+template<typename T>
+typename gnu_obj_2<T>::antiquities gnu_obj_2<T>::value_derived = etruscan;
+
+// Test three.
+template<typename T>
+class gnu_obj_3
+{
+public:
+  typedef region antiquities;
+  static gnu_obj_2<int> data;
       
-    public:
-      gnu_obj_3(antiquities b) { }
-    }; 
+public:
+  gnu_obj_3(antiquities b) { }
+}; 
 
-  template<typename T>
-    gnu_obj_2<int> gnu_obj_3<T>::data(etruscan);
+template<typename T>
+gnu_obj_2<int> gnu_obj_3<T>::data(etruscan);
 
-  // 2002-08-16
-  // Test four.
+// 2002-08-16
+// Test four.
 #include "m-static.h"
-} 
 
 // instantiate templates explicitly so their static members will exist
-template class __gnu_test::gnu_obj_2<int>;
-template class __gnu_test::gnu_obj_2<long>;
-template class __gnu_test::gnu_obj_3<long>;
+template class gnu_obj_2<int>;
+template class gnu_obj_2<long>;
+template class gnu_obj_3<long>;
 
 int main()
 {
-  using namespace __gnu_test;
-
   gnu_obj_1		test1(egyptian, 4589);
   gnu_obj_2<long>	test2(roman);
   gnu_obj_3<long>	test3(greek);
Index: m-static1.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/m-static1.cc,v
retrieving revision 1.1
diff -u -p -r1.1 m-static1.cc
--- m-static1.cc	18 Sep 2002 18:48:43 -0000	1.1
+++ m-static1.cc	17 Dec 2002 23:20:34 -0000
@@ -1,9 +1,5 @@
 // 2002-08-16
 
-namespace __gnu_test {
 #include "m-static.h"
-}
-
-using namespace __gnu_test;
 
 const int gnu_obj_4::elsewhere = 221;
Index: try_catch.cc
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/try_catch.cc,v
retrieving revision 1.1
diff -u -p -r1.1 try_catch.cc
--- try_catch.cc	27 May 2002 20:06:55 -0000	1.1
+++ try_catch.cc	17 Dec 2002 23:20:43 -0000
@@ -4,50 +4,45 @@
 #include <stdexcept>
 #include <string>
 
-namespace __gnu_test
-{
-  enum 	region { oriental, egyptian, greek, etruscan, roman };
+enum region { oriental, egyptian, greek, etruscan, roman };
 
-  // Test one.
-  class gnu_obj_1
-  {
-  public:
-    typedef region antiquities;
-    const bool 		test;
-    const int 		key1;
-    long       		key2;
-
-    antiquities 	value;
-
-    gnu_obj_1(antiquities a, long l): test(true), key1(5), key2(l), value(a) {}
-  };
-
-  // Test two.
-  template<typename T>
-    class gnu_obj_2: public virtual gnu_obj_1
-    {
-    public:
-      antiquities	value_derived;
-      
-      gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7), value_derived(b) { }
-    }; 
-
-  // Test three.
-  template<typename T>
-    class gnu_obj_3
-    {
-    public:
-      typedef region antiquities;
-      gnu_obj_2<int>   	data;
+// Test one.
+class gnu_obj_1
+{
+public:
+  typedef region antiquities;
+  const bool 		test;
+  const int 		key1;
+  long       		key2;
+
+  antiquities 	value;
+
+  gnu_obj_1(antiquities a, long l): test(true), key1(5), key2(l), value(a) {}
+};
+
+// Test two.
+template<typename T>
+class gnu_obj_2: public virtual gnu_obj_1
+{
+public:
+  antiquities	value_derived;
+  
+  gnu_obj_2(antiquities b): gnu_obj_1(oriental, 7), value_derived(b) { }
+}; 
+
+// Test three.
+template<typename T>
+class gnu_obj_3
+{
+public:
+  typedef region antiquities;
+  gnu_obj_2<int>   	data;
       
-      gnu_obj_3(antiquities b): data(etruscan) { }
-    }; 
-} 
+  gnu_obj_3(antiquities b): data(etruscan) { }
+}; 
 
 int main()
 {
-  using namespace __gnu_test;
-
   bool test = true;
   const int i = 5;
   int j = i;
@@ -58,12 +53,12 @@ int main()
   try
     {
       ++j;
-      throw gnu_obj_1(egyptian, 4589);      
+      throw gnu_obj_1(egyptian, 4589);	// marker 1-throw
     }
   catch (gnu_obj_1& obj)
     {
       ++j;
-      if (obj.value != egyptian)
+      if (obj.value != egyptian)	// marker 1-catch
 	test &= false;
       if (obj.key2 != 4589)
 	test &= false;     
@@ -77,19 +72,19 @@ int main()
   // 2
   try
     {
-      ++j;
+      ++j;				// marker 2-start
       try
 	{
-	  ++j;
+	  ++j;				// marker 2-next
 	  try
 	    {
 	      ++j;
-	      throw gnu_obj_1(egyptian, 4589); 
+	      throw gnu_obj_1(egyptian, 4589); // marker 2-throw
 	    }
 	  catch (gnu_obj_1& obj)
 	    {
 	      ++j;
-	      if (obj.value != egyptian)
+	      if (obj.value != egyptian) // marker 2-catch
 		test &= false;
 	      if (obj.key2 != 4589)
 		test &= false;     
@@ -115,11 +110,11 @@ int main()
   try
     {
       if (j < 100)
-	throw invalid_argument("gdb.1");
+	throw invalid_argument("gdb.1"); // marker 3-throw
     }
   catch (exception& obj)
     {
-      if (obj.what() != "gdb.1")
+      if (obj.what() != "gdb.1")	// marker 3-catch
 	test &= false;
     }
   return 0;
Index: try_catch.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/try_catch.exp,v
retrieving revision 1.1
diff -u -p -r1.1 try_catch.exp
--- try_catch.exp	27 May 2002 20:06:55 -0000	1.1
+++ try_catch.exp	17 Dec 2002 23:20:45 -0000
@@ -54,31 +54,29 @@ if ![runto_main] then {
     continue
 }
 
-# One.
+gdb_breakpoint [gdb_get_line_number "marker 1-throw"]
+gdb_continue_to_breakpoint "marker 1-throw"
 
-gdb_test "break 61" "Breakpoint \[0-9\]*.*line 61\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:61\r\n.*" "continue to 61"
+gdb_breakpoint [gdb_get_line_number "marker 1-catch"]
+gdb_continue_to_breakpoint "marker 1-catch"
 
-gdb_test "break 66" "Breakpoint \[0-9\]*.*line 66\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:66\r\n.*" "continue to 66"
+gdb_breakpoint [gdb_get_line_number "marker 2-start"]
+gdb_continue_to_breakpoint "marker 2-start"
 
-gdb_test "break 80" "Breakpoint \[0-9\]*.*line 80\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:80\r\n.*" "continue to 80"
+gdb_breakpoint [gdb_get_line_number "marker 2-next"]
+gdb_continue_to_breakpoint "marker 2-next"
 
-gdb_test "break 83" "Breakpoint \[0-9\]*.*line 83\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:83\r\n.*" "continue to 83"
+gdb_breakpoint [gdb_get_line_number "marker 2-throw"]
+gdb_continue_to_breakpoint "marker 2-throw"
 
-gdb_test "break 87" "Breakpoint \[0-9\]*.*line 87\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:87\r\n.*" "continue to 87"
+gdb_breakpoint [gdb_get_line_number "marker 2-catch"]
+gdb_continue_to_breakpoint "marker 2-catch"
 
-gdb_test "break 92" "Breakpoint \[0-9\]*.*line 92\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:92\r\n.*" "continue to 92"
+gdb_breakpoint [gdb_get_line_number "marker 3-throw"]
+gdb_continue_to_breakpoint "marker 3-throw"
 
-gdb_test "break 118" "Breakpoint \[0-9\]*.*line 118\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:118\r\n.*" "continue to 118"
-
-gdb_test "break 122" "Breakpoint \[0-9\]*.*line 122\\."
-gdb_test "continue" "Continuing\\.\r\n\r\nBreakpoint.*at.*try_catch\\.cc:122\r\n.*" "continue to 122"
+gdb_breakpoint [gdb_get_line_number "marker 3-catch"]
+gdb_continue_to_breakpoint "marker 3-catch"
 
 gdb_exit
 return 0

Re: [patch] delete namespace __gnu_test from C++ testsuite

[Daniel Jacobowitz]

On Tue, Dec 17, 2002 at 03:28:17PM -0800, David Carlton wrote:
> I've committed the following patches to delete the namespace
> __gnu_test from the C++ testsuite files in which it occurs: it's
> pointless, illegally-named, and turns those files into tests of
> namespace support as well as tests of whatever they're trying to test.

I hardly agree.  These tests were taken from libstdc++ or the libstdc++
testsuite, weren't they?  They have to work with the namespace.

I'd rather you added simpler versions rather than changing what an
existing test is testing.  That messes up test results history in
general.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] delete namespace __gnu_test from C++ testsuite

[David Carlton]

On Tue, 17 Dec 2002 18:35:00 -0500, Daniel Jacobowitz <drow@mvista.com> said:
> On Tue, Dec 17, 2002 at 03:28:17PM -0800, David Carlton wrote:

>> I've committed the following patches to delete the namespace
>> __gnu_test from the C++ testsuite files in which it occurs: it's
>> pointless, illegally-named, and turns those files into tests of
>> namespace support as well as tests of whatever they're trying to
>> test.

> I hardly agree.  These tests were taken from libstdc++ or the
> libstdc++ testsuite, weren't they?  They have to work with the
> namespace.

Geez - you were the one who complained about that namespace when I
originally made the patch that created m-static1.cc. :-)

Anyways: I don't know where they were taken from: there's no
indication in the files or the ChangeLogs that they were taken from
anywhere else.  They may be taken from libstdc++, but they certainly
don't depend on libstdc++: the tests are in standard C++, and most of
the tests don't involve the library at all.

> I'd rather you added simpler versions rather than changing what an
> existing test is testing.  That messes up test results history in
> general.

Removing the namespace doesn't change what gdb.sum looks like at all.
If I were to add simpler versions, they would change the current file

  namespace __gnu_test
  {
    <lots of stuff>
  }

  main ()
  {
    using namespace __gnu_test;

    <more stuff>
  }

to

  namespace __gnu_test
  {
    <lots of stuff>
  }

  <the exact same lots of stuff, trivially modified to prevent name clashes>

  main ()
  {
    using namespace __gnu_test;

    <more stuff>

    <the exact same more stuff, trivially modified to prevent name clashes>
  }

(Plus similar changes for the .exp file.)  I don't see what the point
of that is.

Basically, I don't like tests that test multiple concepts at once, as
a general policy: if you have a test testing A and B then, when it
fails, you only know that you screwed up A or B, but not which.
(Exactly such a situation is what prompted me to remove these
namespaces: I'm not doing it for theoretical reasons, but because it's
already caused a practical problem for me once.)

When concepts A and B interact in nontrivial ways, then you should
ideally have tests for A, B, and A+B: and, indeed, m-data.cc, say, has
tests for data members that don't involve templates as well as data
members that do involve templates.  But it previously didn't have any
tests that involved data members but that didn't involve namespaces;
that's bad.  After the patch, it doesn't have tests that involve both
data members and namespaces; that's also bad, but it's not as bad, and
namespace.exp goes a long way towards covering that gap.

Admittedly, namespaces are orthogonal enough to other C++ language
issues that we may eventually want all or almost all of the C++
testsuite files to include namespace tests as well.  But, in the mean
time, I'd rather have focused tests rather than vague tests.

David Carlton
carlton@math.stanford.edu

Re: [patch] delete namespace __gnu_test from C++ testsuite

[David Carlton]

On 17 Dec 2002 16:08:00 -0800, David Carlton <carlton@math.Stanford.EDU> said:

> Basically, I don't like tests that test multiple concepts at once,
> as a general policy: if you have a test testing A and B then, when
> it fails, you only know that you screwed up A or B, but not which.
> (Exactly such a situation is what prompted me to remove these
> namespaces: I'm not doing it for theoretical reasons, but because
> it's already caused a practical problem for me once.)

Actually, I'm misremembering the situation.  What actually happened is
that the tests in question were wrong.  The .cc file looked like this,
among other things:

namespace __gnu_test
{
  enum region { oriental, egyptian, greek, etruscan, roman };
}

Then, at some point, the .exp file asked GDB to print the value of a
variable whose type was that enum, as follows:

# simple object, enum
gdb_test "print test1.value" "\\$\[0-9\]* = egyptian" "simple object, enum"

But this isn't really correct: a more sensible thing to look for is
the value __gnu_test::egyptian: that's the canonical name of the enum
constant in question.  Which is, in fact, what my branch did, causing
tests to go from PASS to FAIL when they should have been going from
FAIL to PASS!

Obviously it would be possible to fix the expected output for the
tests, but then there's the problem that the current GDB wouldn't get
them right (which isn't a big deal, in fact which is probably
desirable, we can KFAIL them) and the more important problem that
we're left without a test that tests GDB's ability to print out data
members whose values are enum constants.

So, for now, I'm a lot more interested in the latter issue, and
removing the __gnu_test seemed like the best way to fix it.  But I'll
make sure I've filed a PR so that I add back tests to test this aspect
of namespace support properly.

David Carlton
carlton@math.stanford.edu