| Message ID | 97d0c4da-2001-af3d-b220-079ac85a46fa@posteo.de |
|---|---|
| State | New |
| Headers | show |
| Series | PR libstdc++/90945 Patch to have pretty printer for std::vector<bool> return bool intead of int for elements | expand |
On 19/06/19 19:04 +0200, Michael Weghorn wrote: >Hi everyone, > >the Python pretty printer for a 'std::vector<bool>' currently returns >integers as values for the elements, which e.g. leads to the situation >that a 'gdb.Value' constructed from that doesn't have 'bool' type, but >an integer type ('long long' for my test with gdb 8.2.1 on Debian >testing, amd64). Returning bool values ('True'/'False') would make sure >that the type is clear and can thus help improve the displayed type. >More details in [1]. > >The attached patch changes the behaviour of the pretty printer accordingly. > > >I'd be glad to receive feedback on this and also notes in case anything >else is needed. (This is my first contribution.) Thanks, the patch looks fine and is small enough that we can accept it without a copyright assignment, but if you plan to contribute again you should look into https://gcc.gnu.org/contribute.html#legal I think I'd prefer to have the 'elt' variable be the actual element (not the unsigned long that contains the element) so I'll adjust the patch to do this instead: elt = bool(self.item.dereference() & (1 << self.so)) >So far, I've tested this with GDB 8.2.1 on Debian testing. It looks like we don't have any tests in the testsuite for printing vector<bool>, so I'll add one to verify this behaviour and commit your patch - thanks! I've attached what I'm testing and plan to commit.
Thank you for the quick reply! On 19/06/2019 21.37, Jonathan Wakely wrote: > Thanks, the patch looks fine and is small enough that we can accept it > without a copyright assignment, but if you plan to contribute again > you should look into https://gcc.gnu.org/contribute.html#legal I'll do as soon as I plan to submit another patch. > I think I'd prefer to have the 'elt' variable be the actual element > (not the unsigned long that contains the element) so I'll adjust the > patch to do this instead: > > elt = bool(self.item.dereference() & (1 << self.so)) > >> So far, I've tested this with GDB 8.2.1 on Debian testing. > > It looks like we don't have any tests in the testsuite for printing > vector<bool>, so I'll add one to verify this behaviour and commit your > patch - thanks! > > I've attached what I'm testing and plan to commit. This sounds all reasonable, just one comment on the test: > + std::vector<bool> vb; > + vb.reserve(100); > + vb.push_back(true); > + vb.push_back(true); > + vb.push_back(false); > + vb.push_back(false); > + vb.push_back(true); > + vb.erase(vb.begin()); > +// { dg-final { regexp-test vb {std::(__debug::)?vector of length 4, capacity 100 = \\{true, false, false, true\\}} } } > + This inserts 5 elements, so I'd expect that either "vector of length 5" and an additional "true" element at the beginning need to be added for the expected result or one of the two first 'vb.push_back(true)' needs to be removed. Thanks again.
On 19/06/19 21:49 +0200, Michael Weghorn wrote: >Thank you for the quick reply! > >On 19/06/2019 21.37, Jonathan Wakely wrote: >> Thanks, the patch looks fine and is small enough that we can accept it >> without a copyright assignment, but if you plan to contribute again >> you should look into https://gcc.gnu.org/contribute.html#legal > >I'll do as soon as I plan to submit another patch. > >> I think I'd prefer to have the 'elt' variable be the actual element >> (not the unsigned long that contains the element) so I'll adjust the >> patch to do this instead: >> >> elt = bool(self.item.dereference() & (1 << self.so)) >> >>> So far, I've tested this with GDB 8.2.1 on Debian testing. >> >> It looks like we don't have any tests in the testsuite for printing >> vector<bool>, so I'll add one to verify this behaviour and commit your >> patch - thanks! >> >> I've attached what I'm testing and plan to commit. > >This sounds all reasonable, just one comment on the test: > >> + std::vector<bool> vb; >> + vb.reserve(100); >> + vb.push_back(true); >> + vb.push_back(true); >> + vb.push_back(false); >> + vb.push_back(false); >> + vb.push_back(true); >> + vb.erase(vb.begin()); >> +// { dg-final { regexp-test vb {std::(__debug::)?vector of length 4, capacity 100 = \\{true, false, false, true\\}} } } >> + > >This inserts 5 elements, so I'd expect that either "vector of length 5" >and an additional "true" element at the beginning need to be added for >the expected result or one of the two first 'vb.push_back(true)' needs >to be removed. It inserts five then erases one, the test is right. Except that it should be capacity=128, because the capacity increases in units of 64 bits. of course.
On 19/06/2019 21.54, Jonathan Wakely wrote: >>> + std::vector<bool> vb; >>> + vb.reserve(100); >>> + vb.push_back(true); >>> + vb.push_back(true); >>> + vb.push_back(false); >>> + vb.push_back(false); >>> + vb.push_back(true); >>> + vb.erase(vb.begin()); >>> +// { dg-final { regexp-test vb {std::(__debug::)?vector of length 4, >>> capacity 100 = \\{true, false, false, true\\}} } } >>> + >> >> This inserts 5 elements, so I'd expect that either "vector of length 5" >> and an additional "true" element at the beginning need to be added for >> the expected result or one of the two first 'vb.push_back(true)' needs >> to be removed. > > It inserts five then erases one, the test is right. > > Except that it should be capacity=128, because the capacity increases > in units of 64 bits. of course. Yes, of course. Sorry, I was missing that.
On 19/06/19 21:58 +0200, Michael Weghorn wrote: >On 19/06/2019 21.54, Jonathan Wakely wrote: >>>> + std::vector<bool> vb; >>>> + vb.reserve(100); >>>> + vb.push_back(true); >>>> + vb.push_back(true); >>>> + vb.push_back(false); >>>> + vb.push_back(false); >>>> + vb.push_back(true); >>>> + vb.erase(vb.begin()); >>>> +// { dg-final { regexp-test vb {std::(__debug::)?vector of length 4, >>>> capacity 100 = \\{true, false, false, true\\}} } } >>>> + >>> >>> This inserts 5 elements, so I'd expect that either "vector of length 5" >>> and an additional "true" element at the beginning need to be added for >>> the expected result or one of the two first 'vb.push_back(true)' needs >>> to be removed. >> >> It inserts five then erases one, the test is right. >> >> Except that it should be capacity=128, because the capacity increases >> in units of 64 bits. of course. > >Yes, of course. Sorry, I was missing that. Here's the patch that I've committed to trunk, after testing. This is probably OK to backport to the release branches as well, so I'll add that to my TODO list. Thanks again for the patch!
On 19/06/2019 21:54, Jonathan Wakely wrote: > On 19/06/19 21:49 +0200, Michael Weghorn wrote: >> On 19/06/2019 21.37, Jonathan Wakely wrote: >>> + std::vector<bool> vb; >>> + vb.reserve(100); >>> + vb.push_back(true); >>> + vb.push_back(true); >>> + vb.push_back(false); >>> + vb.push_back(false); >>> + vb.push_back(true); >>> + vb.erase(vb.begin()); >>> +// { dg-final { regexp-test vb {std::(__debug::)?vector of length 4, >>> capacity 100 = \\{true, false, false, true\\}} } } >>> + >> >> This inserts 5 elements, so I'd expect that either "vector of length 5" >> and an additional "true" element at the beginning need to be added for >> the expected result or one of the two first 'vb.push_back(true)' needs >> to be removed. > > It inserts five then erases one, the test is right. Just one thought that occurred to me while idly browsing this thread: Wouldn't it be better in general to have non-symmetric content to test against, to check that the printer doesn't print it in reverse?
On 20/06/19 08:12 +0200, Stephan Bergmann wrote: >On 19/06/2019 21:54, Jonathan Wakely wrote: >>On 19/06/19 21:49 +0200, Michael Weghorn wrote: >>>On 19/06/2019 21.37, Jonathan Wakely wrote: >>>>+ std::vector<bool> vb; >>>>+ vb.reserve(100); >>>>+ vb.push_back(true); >>>>+ vb.push_back(true); >>>>+ vb.push_back(false); >>>>+ vb.push_back(false); >>>>+ vb.push_back(true); >>>>+ vb.erase(vb.begin()); >>>>+// { dg-final { regexp-test vb {std::(__debug::)?vector of >>>>length 4, capacity 100 = \\{true, false, false, true\\}} } } >>>>+ >>> >>>This inserts 5 elements, so I'd expect that either "vector of length 5" >>>and an additional "true" element at the beginning need to be added for >>>the expected result or one of the two first 'vb.push_back(true)' needs >>>to be removed. >> >>It inserts five then erases one, the test is right. > >Just one thought that occurred to me while idly browsing this thread: >Wouldn't it be better in general to have non-symmetric content to test >against, to check that the printer doesn't print it in reverse? It certainly would, good idea! It's not inconceivable that the bit-shifting code in the printer could be backwards, or affected by endianness. Ideally we'd also test a vector<bool> with more than 64 elements, but I don't have the patience to add it to the test ;-) Tested x86_64-linux, committed to trunk.
From c62cbd47ea8602e2351ff3b0c4c80fa8cb71e313 Mon Sep 17 00:00:00 2001 From: Michael Weghorn <m.weghorn@posteo.de> Date: Wed, 19 Jun 2019 17:22:16 +0200 Subject: [PATCH] Have std::vector printer's iterator return bool Have the pretty-printer for 'std::vector<bool>' return a value of type 'bool' rather than an 'int'. This way, the type is clear and that can be used for better display and a 'gdb.Value' constructed from the returned value will have type 'bool' again, not e.g. 'long long' as happened previously (at least with GDB 8.2.1 on amd64). diff --git a/libstdc++-v3/python/libstdcxx/v6/printers.py b/libstdc++-v3/python/libstdcxx/v6/printers.py index 05143153bee..177db3c0fdb 100644 --- a/libstdc++-v3/python/libstdcxx/v6/printers.py +++ b/libstdc++-v3/python/libstdcxx/v6/printers.py @@ -363,15 +363,12 @@ class StdVectorPrinter: if self.item == self.finish and self.so >= self.fo: raise StopIteration elt = self.item.dereference() - if elt & (1 << self.so): - obit = 1 - else: - obit = 0 + val = elt & (1 << self.so) != 0 self.so = self.so + 1 if self.so >= self.isize: self.item = self.item + 1 self.so = 0 - return ('[%d]' % count, obit) + return ('[%d]' % count, val) else: if self.item == self.finish: raise StopIteration -- 2.20.1
Hi everyone, the Python pretty printer for a 'std::vector<bool>' currently returns integers as values for the elements, which e.g. leads to the situation that a 'gdb.Value' constructed from that doesn't have 'bool' type, but an integer type ('long long' for my test with gdb 8.2.1 on Debian testing, amd64). Returning bool values ('True'/'False') would make sure that the type is clear and can thus help improve the displayed type. More details in [1]. The attached patch changes the behaviour of the pretty printer accordingly. I'd be glad to receive feedback on this and also notes in case anything else is needed. (This is my first contribution.) So far, I've tested this with GDB 8.2.1 on Debian testing. ChangeLog: * PR libstdc++/90945: Have pretty printer return bool instead of int for value of std::vector<bool> elements Regards, Michael [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90945