From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.rbg.tum.de (mailout2.rbg.tum.de [131.159.0.202]) by sourceware.org (Postfix) with ESMTPS id 356733857010; Mon, 12 Sep 2022 08:37:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 356733857010 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=in.tum.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=in.tum.de Received: from mailrelay1.rbg.tum.de (mailrelay1.in.tum.de [131.159.254.14]) by mailout2.rbg.tum.de (Postfix) with ESMTPS id A5B004C01FF; Mon, 12 Sep 2022 10:37:11 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=in.tum.de; s=20220209; t=1662971831; bh=yoP96gVKIxtfPRdmaoy4kpWFt6vEVPMQaSV6inkwecU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=hwTRl3Nzlv+OSw72Ut6p9e0S7cTxjoRVcWXApoS7/lH4DbkY5mtBo8JvZGtfH0BjV qRFmEMBxOvX7b1V0IhjYWT98EtI5imBg/N6wAofOYkf9+8ooDWU2Cp0hSP4LbmiaMa vW84XLKgUn0msXRUqTF+lwG+bu1Oe1lwedo+R33MbZtiTKaY0n31OaznLSdJTrgaUp 3iO1ZaWh/dly3rYyYybe5CWSBcBEG90oFtyStF+g1Bc+UViuRl/fvgB5CPtiZeJePI IsWMZIJigtMNV0ID6bsG3EBekxatBpWiQfa0/qVSGWbkAtHqwwbiDqzBuNjP4jpXRm 77lG3BEynCqIw== Received: by mailrelay1.rbg.tum.de (Postfix, from userid 112) id 9F12C542; Mon, 12 Sep 2022 10:37:11 +0200 (CEST) Received: from mailrelay1.rbg.tum.de (localhost [127.0.0.1]) by mailrelay1.rbg.tum.de (Postfix) with ESMTP id 7216B135; Mon, 12 Sep 2022 10:37:11 +0200 (CEST) Received: from mail.in.tum.de (vmrbg426.in.tum.de [131.159.0.73]) by mailrelay1.rbg.tum.de (Postfix) with ESMTPS id 705AC22; Mon, 12 Sep 2022 10:37:11 +0200 (CEST) Received: by mail.in.tum.de (Postfix, from userid 112) id 6DC004A02F6; Mon, 12 Sep 2022 10:37:11 +0200 (CEST) Received: (Authenticated sender: fent) by mail.in.tum.de (Postfix) with ESMTPSA id 0A3F64A01EE; Mon, 12 Sep 2022 10:37:10 +0200 (CEST) (Extended-Queue-bit xtech_ht@fff.in.tum.de) Message-ID: Date: Mon, 12 Sep 2022 10:37:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH 2/2] libstdc++: Add pretty printer for std::stringstream Content-Language: en-US To: Jonathan Wakely Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org References: <20220904184735.177348-1-fent@in.tum.de> <20220904184735.177348-2-fent@in.tum.de> From: Philipp Fent In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KAM_SHORT,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Jonathan, I've sent an updated patch addressing your comments here: https://gcc.gnu.org/pipermail/libstdc++/2022-September/054587.html Details below. On 06.09.22 13:27, Jonathan Wakely wrote: >> + pbase = self.val['_M_out_beg'] >> + pptr = self.val['_M_out_cur'] >> + egptr = self.val['_M_in_end'] > > It would be nice to encapsulate this into a generic helper for all > streambufs, as this part isn't specific to basic_stringbuf. We could > then reuse it for printing spanbufs and other types in future. It > could be a function that takes a streambuf and returns a tuple of > (pbase, pptr, egptr). I factored this into a reusable access_streambuf_ptrs(). >> + # Logic from basic_stringbuf::_M_high_mark() >> + if pptr: >> + if not egptr or pptr > egptr: >> + return pbase.string(length = pptr - pbase) >> + else: >> + return pbase.string(length = pptr - egptr) > > Shouldn't this be egptr - pbase? > > This suggests the tests are inadequate. I think this bug would have > been found by a test something like: > > std::stringstream ssin("input", std::ios::in); > // { dg-final { note-test ssin "\"input\"" } } > > The (egptr - pbase) case is also needed for printing an istringstream. Good catch! I fixed that bug and added a test for this in the testsuite. > >> + return self.val['_M_string'] >> + >> + def display_hint(self): >> + return 'string' >> + >> +class StdStringStreamPrinter: >> + "Print a std::basic_stringstream" >> + >> + def __init__(self, _, val): >> + self.val = val >> + >> + def to_string(self): >> + return self.val['_M_stringbuf'] > > This assumes that the stream is still using its stringbuf, which is > probably true 99% of the time, but isn't guaranteed. In the following > situation, the printer would give potentially misleading output: > > std::stringstream ss("xxx"); > ss.rdbuf(std::cin.rdbuf()); > > I wonder if we want to have a check that self.val['_M_streambuf'] == > self.val['_M_stringbuf'].address > > Alternatively, don't provide a printer for the stringstream at all, > just for the stringbuf, and then when GDB uses its default display for > the stringstream it will show that member. I didn't know one could redirect a stringstream, since its rdbuf() method hides the basic_ios rdbuf() methods. But of course, that's still possible via the base class... The Patch v2 checks that case in the StdStringStreamPrinter constructor, and follows that redirect in to_string(). > >> + >> + def display_hint(self): >> + return 'string' >> + >> class Tr1HashtableIterator(Iterator): >> def __init__ (self, hashtable): >> self.buckets = hashtable['_M_buckets'] >> @@ -2232,6 +2265,10 @@ def build_libstdcxx_dictionary (): >> libstdcxx_printer.add_version('std::', 'initializer_list', >> StdInitializerListPrinter) >> libstdcxx_printer.add_version('std::', 'atomic', StdAtomicPrinter) >> + libstdcxx_printer.add_version('std::', 'basic_stringbuf', StdStringBufPrinter) >> + libstdcxx_printer.add_version('std::__cxx11::', 'basic_stringbuf', StdStringBufPrinter) >> + libstdcxx_printer.add_version('std::', 'basic_stringstream', StdStringStreamPrinter) >> + libstdcxx_printer.add_version('std::__cxx11::', 'basic_stringstream', StdStringStreamPrinter) > > Wouldn't we want it for ostringstream and istringstream too? Indeed. The updated patch registers the pretty printer for all flavors.