Message ID | 1474949330-4307-2-git-send-email-tom@tromey.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 97665 invoked by alias); 27 Sep 2016 04:48:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 97524 invoked by uid 89); 27 Sep 2016 04:48:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=no version=3.3.2 spammy=H*RU:10.0.90.82, Hx-spam-relays-external:CMOut01, H*r:CMOut01, H*RU:CMOut01 X-HELO: gproxy4-pub.mail.unifiedlayer.com Received: from gproxy4-pub.mail.unifiedlayer.com (HELO gproxy4-pub.mail.unifiedlayer.com) (69.89.23.142) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with SMTP; Tue, 27 Sep 2016 04:48:02 +0000 Received: (qmail 10137 invoked by uid 0); 27 Sep 2016 04:48:01 -0000 Received: from unknown (HELO CMOut01) (10.0.90.82) by gproxy4.mail.unifiedlayer.com with SMTP; 27 Sep 2016 04:48:01 -0000 Received: from box522.bluehost.com ([74.220.219.122]) by CMOut01 with id oUnw1t00m2f2jeq01UnzXH; Mon, 26 Sep 2016 22:47:59 -0600 X-Authority-Analysis: v=2.1 cv=OPq0g0qB c=1 sm=1 tr=0 a=GsOEXm/OWkKvwdLVJsfwcA==:117 a=GsOEXm/OWkKvwdLVJsfwcA==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=GW1xBdLrtEIA:10 a=zstS-IiYAAAA:8 a=qUXFsv20AAAA:8 a=qP7Aq_7qRByOJlosDe4A:9 a=r7gNwwvMS7Ivrw7E:21 a=Rkn0clDWT2CNkvnJ:21 a=4G6NA9xxw8l3yy4pmD5M:22 a=9MODbo6-FXLeArg8YEg9:22 Received: from 71-218-192-86.hlrn.qwest.net ([71.218.192.86]:56110 helo=bapiya.Home) by box522.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.86_1) (envelope-from <tom@tromey.com>) id 1bojhM-0006Nj-RT; Mon, 26 Sep 2016 22:08:52 -0600 From: Tom Tromey <tom@tromey.com> To: gdb-patches@sourceware.org Cc: Tom Tromey <tom@tromey.com> Subject: [RFA 01/22] Change selttest.c to use use std::vector Date: Mon, 26 Sep 2016 22:08:29 -0600 Message-Id: <1474949330-4307-2-git-send-email-tom@tromey.com> In-Reply-To: <1474949330-4307-1-git-send-email-tom@tromey.com> References: <1474949330-4307-1-git-send-email-tom@tromey.com> X-BWhitelist: no X-Exim-ID: 1bojhM-0006Nj-RT X-Source-Sender: 71-218-192-86.hlrn.qwest.net (bapiya.Home) [71.218.192.86]:56110 X-Source-Auth: tom+tromey.com X-Email-Count: 23 X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTIyLmJsdWVob3N0LmNvbQ== |
Commit Message
Tom Tromey
Sept. 27, 2016, 4:08 a.m. UTC
This patch changes selftest.c to use std::vector rather than VEC. I think this is a small net plus. Because we're using C++03, we unfortunately don't all the possible benefits here; namely, iterating over a vector is still a pain. For reference, in C++11 the loop would be just: for (auto f : tests) ... 2016-09-26 Tom Tromey <tom@tromey.com> * selftest.c: Include <vector>, not "vec.h". (self_test_function_ptr): Remove. (tests): Now a std::vector. (register_self_test, run_self_tests): Update. --- gdb/ChangeLog | 7 +++++++ gdb/selftest.c | 22 +++++++++------------- 2 files changed, 16 insertions(+), 13 deletions(-)
Comments
On Mon, Sep 26, 2016 at 10:08:29PM -0600, Tom Tromey wrote: > This patch changes selftest.c to use std::vector rather than VEC. > > I think this is a small net plus. Because we're using C++03, we I'd agree, hopefully we can throw VEC in the trash some day. Relatedly we should probably move gcc's hash table stuff to include/ sooner rather than later so we can similarly get rid of htab. > unfortunately don't all the possible benefits here; namely, iterating I think you missed get in that sentence. > over a vector is still a pain. For reference, in C++11 the loop would > be just: > > for (auto f : tests) its debatable, but imho its not necessarily obvious what the type of the local is when you use auto to iterate over vectors especially when they are members or globals like here. > +static std::vector<self_test_function *> tests; should we use a pointer to avoid the static initializer? > - for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i) > + for (std::vector<self_test_function *>::iterator iter = tests.begin (); > + iter != tests.end (); > + ++iter) I believe you can "cheat" here and just use the function pointer type, because the sane implementation of iterators over vectors is pointers. Trev
>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes: Trevor> I'd agree, hopefully we can throw VEC in the trash some day. Relatedly Trevor> we should probably move gcc's hash table stuff to include/ sooner rather Trevor> than later so we can similarly get rid of htab. That would be nice; though we could probably use std::set and std::map in gdb as well. One wrinkle with hash tables is that they're sometimes allocated on obstacks; would gcc's handle this? >> for (auto f : tests) Trevor> its debatable, but imho its not necessarily obvious what the type of the Trevor> local is when you use auto to iterate over vectors especially when they Trevor> are members or globals like here. Yeah, that's one drawback. Often one actually wants "auto &f" in there. >> +static std::vector<self_test_function *> tests; Trevor> should we use a pointer to avoid the static initializer? I was on the fence about this one. On the one hand, static initializers can be very bad. On the other hand, this one in particular doesn't seem like it could cause problems. >> - for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i) >> + for (std::vector<self_test_function *>::iterator iter = tests.begin (); >> + iter != tests.end (); >> + ++iter) Trevor> I believe you can "cheat" here and just use the function pointer type, Trevor> because the sane implementation of iterators over vectors is pointers. Yeah, or just loop using an int and index into the vector. I'll see if that looks cleaner. Tom
On Tue, Sep 27, 2016 at 09:55:28AM -0600, Tom Tromey wrote: > >>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes: > > Trevor> I'd agree, hopefully we can throw VEC in the trash some day. Relatedly > Trevor> we should probably move gcc's hash table stuff to include/ sooner rather > Trevor> than later so we can similarly get rid of htab. > > That would be nice; though we could probably use std::set and std::map > in gdb as well. One wrinkle with hash tables is that they're sometimes > allocated on obstacks; would gcc's handle this? No, they don't support that at the moment, though I suppose it would be fine for keys or values to point into obstacks. Is there a good reason for this other than using obstacks to provide a sort of automatic memory management? > >> for (auto f : tests) > > Trevor> its debatable, but imho its not necessarily obvious what the type of the > Trevor> local is when you use auto to iterate over vectors especially when they > Trevor> are members or globals like here. > > Yeah, that's one drawback. > Often one actually wants "auto &f" in there. yeah, and not having that can sometimes be bad. > >> +static std::vector<self_test_function *> tests; > > Trevor> should we use a pointer to avoid the static initializer? > > I was on the fence about this one. > On the one hand, static initializers can be very bad. > On the other hand, this one in particular doesn't seem like it could > cause problems. yeah, I think its just zeroing some memory and then registering a static destructor. So its silly that its necessary at all, but whether to avoid it or not is just a perf question. Trev > >> - for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i) > >> + for (std::vector<self_test_function *>::iterator iter = tests.begin (); > >> + iter != tests.end (); > >> + ++iter) > > Trevor> I believe you can "cheat" here and just use the function pointer type, > Trevor> because the sane implementation of iterators over vectors is pointers. > > Yeah, or just loop using an int and index into the vector. > I'll see if that looks cleaner. > > Tom
>>>>> "Trevor" == Trevor Saunders <tbsaunde@tbsaunde.org> writes:
Tom> That would be nice; though we could probably use std::set and std::map
Tom> in gdb as well. One wrinkle with hash tables is that they're sometimes
Tom> allocated on obstacks; would gcc's handle this?
Trevor> No, they don't support that at the moment, though I suppose it
Trevor> would be fine for keys or values to point into obstacks. Is
Trevor> there a good reason for this other than using obstacks to
Trevor> provide a sort of automatic memory management?
Sometimes it's just to avoid more work -- cleaning up an obstack is
easy. E.g., see dwarf2loc.c:func_verify_no_selftailcall Uses like this
can easily be replaced.
There are also spots that allocate a hash table on the objfile obstack.
Now, this is a bit unfortunate because it means that hash table resizes
will create a kind of "leak" -- not a true leak (the memory is tracked
and will be freed), but basically some chunk of memory will no longer be
useful.
It's possible to address these cases in other ways, though more
difficult.
Tom
On 2016-09-27 00:08, Tom Tromey wrote: > - printf_filtered (_("Ran %u unit tests, %d failed\n"), > - VEC_length (self_test_function_ptr, tests), failed); > + printf_filtered (_("Ran %lu unit tests, %d failed\n"), > + tests.size (), failed); This doesn't build on 32 bits: selftest.c: In function ‘void run_self_tests()’: selftest.c:62:27: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 2 has type ‘std::vector<void (*)()>::size_type {aka unsigned int}’ [-Werror=format=] tests.size (), failed); ^ I think the right format size specifier would be 'z'.
>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: >> + printf_filtered (_("Ran %lu unit tests, %d failed\n"), >> + tests.size (), failed); Simon> This doesn't build on 32 bits: Thanks for trying that. I will send it through buildbot before the next round. Simon> I think the right format size specifier would be 'z'. I will probably cast to long instead. I don't see other uses of %zd in gdb. Tom
On 09/27/2016 04:55 PM, Tom Tromey wrote: > Trevor> I'd agree, hopefully we can throw VEC in the trash some day. Relatedly > Trevor> we should probably move gcc's hash table stuff to include/ sooner rather > Trevor> than later so we can similarly get rid of htab. > > That would be nice; though we could probably use std::set and std::map > in gdb as well. std::unordered_map (hash table) would be a more direct replacement. See gold/system.h -- for C++03 compilers, gold uses unordered_map (and unordered_set) from std::tr1 if available, otherwise falls back to std::map (balanced binary tree, usually red/black), with even older compilers. The idea being that you'll be able to use gold with older compilers, though it'll run slower. Thanks, Pedro Alves
On 09/27/2016 04:55 PM, Tom Tromey wrote: > Trevor> should we use a pointer to avoid the static initializer? > > I was on the fence about this one. > On the one hand, static initializers can be very bad. > On the other hand, this one in particular doesn't seem like it could > cause problems. I think it's OK for now. The patch LGTM. We may want to revisit later. I guess with "can be very bad" you're thinking of order of constructors between compilation units. Worth keeping in mind is the not making the library use case harder to get at in the future, considering the potential need for controlled bring up (static ctors) and teardown (static dtors) independent of exit() time. Yet another aspect worth considering is the startup time overhead impact of global constructors. VEC has a POD base class which allows avoiding that: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01651.html See LLVM here considering/doing similar things: https://llvm.org/bugs/show_bug.cgi?id=11944 But that feels like premature optimization too me at this point. Thanks, Pedro Alves
On 09/30/2016 10:16 PM, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes: >>> + printf_filtered (_("Ran %lu unit tests, %d failed\n"), >>> + tests.size (), failed); > > Simon> This doesn't build on 32 bits: > > Thanks for trying that. > I will send it through buildbot before the next round. > > Simon> I think the right format size specifier would be 'z'. > > I will probably cast to long instead. > I don't see other uses of %zd in gdb. That's because the %z modifier is a C99 / C++11 feature. Thanks, Pedro Alves
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a9ef3ee..2f28e54 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,10 @@ +2016-09-26 Tom Tromey <tom@tromey.com> + + * selftest.c: Include <vector>, not "vec.h". + (self_test_function_ptr): Remove. + (tests): Now a std::vector. + (register_self_test, run_self_tests): Update. + 2016-09-23 Jon Turney <jon.turney@dronecode.org.uk> * windows-nat.c (windows_delete_thread): Adjusting call to diff --git a/gdb/selftest.c b/gdb/selftest.c index c63c06d..bbd37b5 100644 --- a/gdb/selftest.c +++ b/gdb/selftest.c @@ -18,22 +18,18 @@ #include "defs.h" #include "selftest.h" -#include "vec.h" - -typedef self_test_function *self_test_function_ptr; - -DEF_VEC_P (self_test_function_ptr); +#include <vector> /* All the tests that have been registered. */ -static VEC (self_test_function_ptr) *tests; +static std::vector<self_test_function *> tests; /* See selftest.h. */ void register_self_test (self_test_function *function) { - VEC_safe_push (self_test_function_ptr, tests, function); + tests.push_back (function); } /* See selftest.h. */ @@ -41,17 +37,17 @@ register_self_test (self_test_function *function) void run_self_tests (void) { - int i; - self_test_function_ptr func; int failed = 0; - for (i = 0; VEC_iterate (self_test_function_ptr, tests, i, func); ++i) + for (std::vector<self_test_function *>::iterator iter = tests.begin (); + iter != tests.end (); + ++iter) { QUIT; TRY { - (*func) (); + (*iter) (); } CATCH (ex, RETURN_MASK_ERROR) { @@ -62,6 +58,6 @@ run_self_tests (void) END_CATCH } - printf_filtered (_("Ran %u unit tests, %d failed\n"), - VEC_length (self_test_function_ptr, tests), failed); + printf_filtered (_("Ran %lu unit tests, %d failed\n"), + tests.size (), failed); }