Hi, Consider the test-case contained in this patch. With g++ (7.5.0) we have for "ptype A": ... type = class A { public: int a; A(void); ~A(); } ... and with clang++ (13.0.1): ... type = class A { public: int a; A(void); ~A(void); } ... and we observe that the destructor is printed differently. There's a difference in debug info between the two cases: in the clang case, there's one artificial parameter, but in the g++ case, there are two, and these similar cases are handled differently in cp_type_print_method_args. This is due to this slightly convoluted bit of code: ... i = staticp ? 0 : 1; if (nargs > i) { while (i < nargs) ... } else if (varargs) gdb_printf (stream, "..."); else if (language == language_cplus) gdb_printf (stream, "void"); ... The purpose of "i = staticp ? 0 : 1" is to skip the printing of the implicit this parameter. In commit 5f4d1085085 ("c++/8218: Destructors w/arguments"), skipping of other artificial parameters was added, but using a different method: rather than adjusting the potential loop start, it skips the parameter in the loop. The observed difference in printing is explained by whether we enter the loop: - in the clang case, the loop is not entered and we print "void". - in the gcc case, the loop is entered, and nothing is printed. Fix this by rewriting the code to: - always enter the loop - handle whether arguments need printing in the loop - keep track of how many arguments are printed, and use that after the loop to print void etc. such that we have the same for both gcc and clang: ... A(void); ~A(void); ... Note that I consider the discussion of whether we want to print: - A(void) / ~A(void), or - A() / ~A() out-of-scope for this patch. Tested on x86_64-linux. Any comments? Thanks, - Tom [gdb/c++] Print destructor the same for gcc and clang --- gdb/c-typeprint.c | 55 +++++++++++++++++------------- gdb/testsuite/gdb.base/ptype-offsets.exp | 2 +- gdb/testsuite/gdb.cp/print-method-args.cc | 38 +++++++++++++++++++++ gdb/testsuite/gdb.cp/print-method-args.exp | 36 +++++++++++++++++++ gdb/testsuite/gdb.cp/templates.exp | 4 +-- 5 files changed, 109 insertions(+), 26 deletions(-) diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c index 3a611cdac5d..23e8a5a4d6f 100644 --- a/gdb/c-typeprint.c +++ b/gdb/c-typeprint.c @@ -273,35 +273,44 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, language_cplus, DMGL_ANSI); gdb_puts ("(", stream); - /* Skip the class variable. We keep this here to accommodate older - compilers and debug formats which may not support artificial - parameters. */ - i = staticp ? 0 : 1; - if (nargs > i) + int printed_args = 0; + for (i = 0; i < nargs; ++i) { - while (i < nargs) + if (i == 0 && !staticp) { - struct field arg = args[i++]; - - /* Skip any artificial arguments. */ - if (FIELD_ARTIFICIAL (arg)) - continue; + /* Skip the class variable. We keep this here to accommodate older + compilers and debug formats which may not support artificial + parameters. */ + continue; + } - c_print_type (arg.type (), "", stream, 0, 0, language, flags); + struct field arg = args[i]; + /* Skip any artificial arguments. */ + if (FIELD_ARTIFICIAL (arg)) + continue; - if (i == nargs && varargs) - gdb_printf (stream, ", ..."); - else if (i < nargs) - { - gdb_printf (stream, ", "); - stream->wrap_here (8); - } + if (printed_args > 0) + { + gdb_printf (stream, ", "); + stream->wrap_here (8); } + + c_print_type (arg.type (), "", stream, 0, 0, language, flags); + printed_args++; + } + + if (varargs) + { + if (printed_args == 0) + gdb_printf (stream, "..."); + else + gdb_printf (stream, ", ..."); + } + else if (printed_args == 0) + { + if (language == language_cplus) + gdb_printf (stream, "void"); } - else if (varargs) - gdb_printf (stream, "..."); - else if (language == language_cplus) - gdb_printf (stream, "void"); gdb_printf (stream, ")"); diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp index e80d876fa32..eb41bafb3e6 100644 --- a/gdb/testsuite/gdb.base/ptype-offsets.exp +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp @@ -104,7 +104,7 @@ gdb_test "ptype /oTM struct abc" \ "/* 48 | 2 */ my_int_type field9;" \ "" \ " abc(void);" \ -" ~abc();" \ +" ~abc(void);" \ "" \ " typedef short my_int_type;" \ "/* XXX 6-byte padding */" \ diff --git a/gdb/testsuite/gdb.cp/print-method-args.cc b/gdb/testsuite/gdb.cp/print-method-args.cc new file mode 100644 index 00000000000..98be8286aeb --- /dev/null +++ b/gdb/testsuite/gdb.cp/print-method-args.cc @@ -0,0 +1,38 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright (C) 2022 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +class A { +public: + int a; + + A() { + this->a = 3; + } + ~A() { + a = 0; + } +}; + +int +main (void) +{ + A a; + + return a.a; +} diff --git a/gdb/testsuite/gdb.cp/print-method-args.exp b/gdb/testsuite/gdb.cp/print-method-args.exp new file mode 100644 index 00000000000..7df044711b0 --- /dev/null +++ b/gdb/testsuite/gdb.cp/print-method-args.exp @@ -0,0 +1,36 @@ +# Copyright (C) 2022 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# This file is part of the gdb testsuite. + +if { [skip_cplus_tests] } { continue } + +standard_testfile .cc + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { + return -1 +} + +set re \ + [multi_line \ + "type = class A {" \ + " public:" \ + " int a;" \ + "" \ + " A\\(void\\);" \ + " ~A\\(void\\);" \ + "}"] + +gdb_test "ptype A" $re diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp index 27c9bbc1c7e..d3ede5a5fb6 100644 --- a/gdb/testsuite/gdb.cp/templates.exp +++ b/gdb/testsuite/gdb.cp/templates.exp @@ -68,7 +68,7 @@ proc test_ptype_of_templates {} { # The destructor has an argument type. kfail "gdb/8218" "ptype T5<int>" } - -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { + -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { pass "ptype T5<int>" } } @@ -108,7 +108,7 @@ proc test_ptype_of_templates {} { # The destructor has an argument type. kfail "gdb/8218" "ptype t5i" } - -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { + -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { pass "ptype t5i" } }
On 27/09/2022 12:35, Tom de Vries wrote: > Hi, > > Consider the test-case contained in this patch. > > With g++ (7.5.0) we have for "ptype A": > ... > type = class A { > public: > int a; > > A(void); > ~A(); > } > ... > and with clang++ (13.0.1): > ... > type = class A { > public: > int a; > > A(void); > ~A(void); > } > ... > and we observe that the destructor is printed differently. > > There's a difference in debug info between the two cases: in the clang case, > there's one artificial parameter, but in the g++ case, there are two, and > these similar cases are handled differently in cp_type_print_method_args. > > This is due to this slightly convoluted bit of code: > ... > i = staticp ? 0 : 1; > if (nargs > i) > { > while (i < nargs) > ... > } > else if (varargs) > gdb_printf (stream, "..."); > else if (language == language_cplus) > gdb_printf (stream, "void"); > ... > > The purpose of "i = staticp ? 0 : 1" is to skip the printing of the implicit > this parameter. > > In commit 5f4d1085085 ("c++/8218: Destructors w/arguments"), skipping of other > artificial parameters was added, but using a different method: rather than > adjusting the potential loop start, it skips the parameter in the loop. > > The observed difference in printing is explained by whether we enter the loop: > - in the clang case, the loop is not entered and we print "void". > - in the gcc case, the loop is entered, and nothing is printed. > > Fix this by rewriting the code to: > - always enter the loop > - handle whether arguments need printing in the loop > - keep track of how many arguments are printed, and > use that after the loop to print void etc. > such that we have the same for both gcc and clang: > ... > A(void); > ~A(void); > ... > > Note that I consider the discussion of whether we want to print: > - A(void) / ~A(void), or > - A() / ~A() > out-of-scope for this patch. > > Tested on x86_64-linux. > > Any comments? > > Thanks, > - Tom > > [gdb/c++] Print destructor the same for gcc and clang Hi Tom, I am ok with these changes, they make the code easier to read and easier to change, based on the output of the A()/A(void) discussion. Cheers, Bruno > --- > gdb/c-typeprint.c | 55 +++++++++++++++++------------- > gdb/testsuite/gdb.base/ptype-offsets.exp | 2 +- > gdb/testsuite/gdb.cp/print-method-args.cc | 38 +++++++++++++++++++++ > gdb/testsuite/gdb.cp/print-method-args.exp | 36 +++++++++++++++++++ > gdb/testsuite/gdb.cp/templates.exp | 4 +-- > 5 files changed, 109 insertions(+), 26 deletions(-) > > diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c > index 3a611cdac5d..23e8a5a4d6f 100644 > --- a/gdb/c-typeprint.c > +++ b/gdb/c-typeprint.c > @@ -273,35 +273,44 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, > language_cplus, DMGL_ANSI); > gdb_puts ("(", stream); > > - /* Skip the class variable. We keep this here to accommodate older > - compilers and debug formats which may not support artificial > - parameters. */ > - i = staticp ? 0 : 1; > - if (nargs > i) > + int printed_args = 0; > + for (i = 0; i < nargs; ++i) > { > - while (i < nargs) > + if (i == 0 && !staticp) > { > - struct field arg = args[i++]; > - > - /* Skip any artificial arguments. */ > - if (FIELD_ARTIFICIAL (arg)) > - continue; > + /* Skip the class variable. We keep this here to accommodate older > + compilers and debug formats which may not support artificial > + parameters. */ > + continue; > + } > > - c_print_type (arg.type (), "", stream, 0, 0, language, flags); > + struct field arg = args[i]; > + /* Skip any artificial arguments. */ > + if (FIELD_ARTIFICIAL (arg)) > + continue; > > - if (i == nargs && varargs) > - gdb_printf (stream, ", ..."); > - else if (i < nargs) > - { > - gdb_printf (stream, ", "); > - stream->wrap_here (8); > - } > + if (printed_args > 0) > + { > + gdb_printf (stream, ", "); > + stream->wrap_here (8); > } > + > + c_print_type (arg.type (), "", stream, 0, 0, language, flags); > + printed_args++; > + } > + > + if (varargs) > + { > + if (printed_args == 0) > + gdb_printf (stream, "..."); > + else > + gdb_printf (stream, ", ..."); > + } > + else if (printed_args == 0) > + { > + if (language == language_cplus) > + gdb_printf (stream, "void"); > } > - else if (varargs) > - gdb_printf (stream, "..."); > - else if (language == language_cplus) > - gdb_printf (stream, "void"); > > gdb_printf (stream, ")"); > > diff --git a/gdb/testsuite/gdb.base/ptype-offsets.exp b/gdb/testsuite/gdb.base/ptype-offsets.exp > index e80d876fa32..eb41bafb3e6 100644 > --- a/gdb/testsuite/gdb.base/ptype-offsets.exp > +++ b/gdb/testsuite/gdb.base/ptype-offsets.exp > @@ -104,7 +104,7 @@ gdb_test "ptype /oTM struct abc" \ > "/* 48 | 2 */ my_int_type field9;" \ > "" \ > " abc(void);" \ > -" ~abc();" \ > +" ~abc(void);" \ > "" \ > " typedef short my_int_type;" \ > "/* XXX 6-byte padding */" \ > diff --git a/gdb/testsuite/gdb.cp/print-method-args.cc b/gdb/testsuite/gdb.cp/print-method-args.cc > new file mode 100644 > index 00000000000..98be8286aeb > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/print-method-args.cc > @@ -0,0 +1,38 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2022 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +class A { > +public: > + int a; > + > + A() { > + this->a = 3; > + } > + ~A() { > + a = 0; > + } > +}; > + > +int > +main (void) > +{ > + A a; > + > + return a.a; > +} > diff --git a/gdb/testsuite/gdb.cp/print-method-args.exp b/gdb/testsuite/gdb.cp/print-method-args.exp > new file mode 100644 > index 00000000000..7df044711b0 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/print-method-args.exp > @@ -0,0 +1,36 @@ > +# Copyright (C) 2022 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +# This file is part of the gdb testsuite. > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { > + return -1 > +} > + > +set re \ > + [multi_line \ > + "type = class A {" \ > + " public:" \ > + " int a;" \ > + "" \ > + " A\\(void\\);" \ > + " ~A\\(void\\);" \ > + "}"] > + > +gdb_test "ptype A" $re > diff --git a/gdb/testsuite/gdb.cp/templates.exp b/gdb/testsuite/gdb.cp/templates.exp > index 27c9bbc1c7e..d3ede5a5fb6 100644 > --- a/gdb/testsuite/gdb.cp/templates.exp > +++ b/gdb/testsuite/gdb.cp/templates.exp > @@ -68,7 +68,7 @@ proc test_ptype_of_templates {} { > # The destructor has an argument type. > kfail "gdb/8218" "ptype T5<int>" > } > - -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > + -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > pass "ptype T5<int>" > } > } > @@ -108,7 +108,7 @@ proc test_ptype_of_templates {} { > # The destructor has an argument type. > kfail "gdb/8218" "ptype t5i" > } > - -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > + -re "type = class T5<int> \{${ws}public:${ws}static int X;${ws}int x;${ws}int val;${ws}T5\\(int\\);${ws}T5\\((T5<int> const|const T5<int>) ?&\\);${ws}~T5\\(void\\);${ws}static void \\* operator new\\((size_t|unsigned( int| long|))\\);${ws}static void operator delete\\(void ?\\*\\);${ws}int value\\((void|)\\);${ws}\}\r\n$gdb_prompt $" { > pass "ptype t5i" > } > } >
On 9/27/22 03:35, Tom de Vries wrote:
>
> Fix this by rewriting the code to:
> - always enter the loop
> - handle whether arguments need printing in the loop
> - keep track of how many arguments are printed, and
> use that after the loop to print void etc.
> such that we have the same for both gcc and clang:
> ...
> A(void);
> ~A(void);
> ...
>
> Note that I consider the discussion of whether we want to print:
> - A(void) / ~A(void), or
> - A() / ~A()
> out-of-scope for this patch.
>
> Tested on x86_64-linux.
>
> Any comments?
This all looks okay; thank you for that cleanup. Consistency is welcome.
On the [off-]topic of "void" vs ""... The standard (IIUC) permits the use of
"void" in destructors. If I create a class with a private dtor, e.g., to elicit
a compilation failure, clang++ never outputs the symbol name. GCC, on the other
hand, does, and it outputs "" (no "void", even if I explicitly use "void" in the
dtor).
While I would prefer "", I don't have a compelling argument either way. [It's
trivial to implement with your patch.]
I recommend you approve your patch. :-)
Keith
On 2022-09-27 11:35 a.m., Tom de Vries wrote: > > Note that I consider the discussion of whether we want to print: > - A(void) / ~A(void), or > - A() / ~A() > out-of-scope for this patch. > I'd vote to remove the void. I can't think of a good reason to keep it (for C++). It just looks odd to C++ developers, IMHO. I'm fine with that being a separate decision from this patch, though. > diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c > index 3a611cdac5d..23e8a5a4d6f 100644 > --- a/gdb/c-typeprint.c > +++ b/gdb/c-typeprint.c > @@ -273,35 +273,44 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, > language_cplus, DMGL_ANSI); > gdb_puts ("(", stream); > > - /* Skip the class variable. We keep this here to accommodate older > - compilers and debug formats which may not support artificial > - parameters. */ > - i = staticp ? 0 : 1; > - if (nargs > i) > + int printed_args = 0; > + for (i = 0; i < nargs; ++i) > { > - while (i < nargs) > + if (i == 0 && !staticp) > { > - struct field arg = args[i++]; > - > - /* Skip any artificial arguments. */ > - if (FIELD_ARTIFICIAL (arg)) > - continue; > + /* Skip the class variable. We keep this here to accommodate older > + compilers and debug formats which may not support artificial > + parameters. */ > + continue; > + } A remark on this preexisting "Skip the class variable." logic -- I strongly suspect that we'll need to revisit it for the C++23 "deducing this" feature: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p0847r6.html With that feature, some methods we'll have explicit, named "this" parameters, and thus, I'd think, be non-artificial. E.g., from the proposal: struct X { void foo(this X const& self, int i); template <typename Self> void bar(this Self&& self); }; Plainly skipping printing the "self" parameter in these methods just because it is a non-static method, and "self" is the first parameter, will be incorrect. According to: https://gcc.gnu.org/projects/cxx-status.html https://clang.llvm.org/cxx_status.html neither GCC not Clang support that yet though. We don't have tests for many of the C++14, C++17 and C++20 features, so I'm not holding my breath waiting for tests for this, though. :-P > +++ b/gdb/testsuite/gdb.cp/print-method-args.exp > @@ -0,0 +1,36 @@ > +# Copyright (C) 2022 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +# This file is part of the gdb testsuite. > + Can you add an intro comment? LGTM with that fixed.
On 2022-09-29 6:52 p.m., Keith Seitz via Gdb-patches wrote: > This all looks okay; thank you for that cleanup. Consistency is welcome. > +1 > On the [off-]topic of "void" vs ""... The standard (IIUC) permits the use of > "void" in destructors. If I create a class with a private dtor, e.g., to elicit > a compilation failure, clang++ never outputs the symbol name. GCC, on the other > hand, does, and it outputs "" (no "void", even if I explicitly use "void" in the > dtor). > > While I would prefer "", I don't have a compelling argument either way. [It's > trivial to implement with your patch.] I would prefer "" too. It just looks weird/redundant for C++, IMHO. > > I recommend you approve your patch. :-) Thanks Keith. That certainly helps!