From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40924 invoked by alias); 17 Jan 2017 19:57:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 40914 invoked by uid 89); 17 Jan 2017 19:57:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=HX-PHP-Originating-Script:rcube.php, H*u:1.2.3, H*UA:1.2.3 X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Jan 2017 19:57:12 +0000 Received: by simark.ca (Postfix, from userid 33) id CFFA11E163; Tue, 17 Jan 2017 14:57:09 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH 5/5] Eliminate make_cleanup_ui_file_delete / make ui_file a class hierarchy X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 17 Jan 2017 19:57:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1484617147-2506-6-git-send-email-palves@redhat.com> References: <1484617147-2506-1-git-send-email-palves@redhat.com> <1484617147-2506-6-git-send-email-palves@redhat.com> Message-ID: <5533317a1fdb2e5f36e64731aef84993@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00335.txt.bz2 I've only looked at ui-file.h/c so far, but overall it looks really nice. On 2017-01-16 20:39, Pedro Alves wrote: > -static void > -null_file_fputs (const char *buf, struct ui_file *file) > -{ > - if (file->to_write == null_file_write) > - /* Both the write and fputs methods are null. Discard the > - request. */ > - return; > - else > - { > - /* The write method was implemented, use that. */ > - file->to_write (file, buf, strlen (buf)); > - } > -} > + Is the insertion of a form feed character on purpose? I know it's mentioned in the GNU coding style, but I was wondering if they were useful to anybody today. > diff --git a/gdb/ui-file.h b/gdb/ui-file.h > index 4ad3940..f1951e4 100644 > --- a/gdb/ui-file.h > +++ b/gdb/ui-file.h > @@ -24,74 +24,86 @@ struct ui_file; The forward declarations at the beginning of the file can go. > > #include > > -/* Create a generic ui_file object with null methods. */ > - > -extern struct ui_file *ui_file_new (void); > - > -/* Override methods used by specific implementations of a UI_FILE > - object. */ > - > -typedef void (ui_file_flush_ftype) (struct ui_file *stream); > -extern void set_ui_file_flush (struct ui_file *stream, > - ui_file_flush_ftype *flush); > - > -/* NOTE: Both fputs and write methods are available. Default > - implementations that mapping one onto the other are included. */ > -typedef void (ui_file_write_ftype) (struct ui_file *stream, > - const char *buf, long length_buf); > -extern void set_ui_file_write (struct ui_file *stream, > - ui_file_write_ftype *fputs); > - > -typedef void (ui_file_fputs_ftype) (const char *, struct ui_file > *stream); > -extern void set_ui_file_fputs (struct ui_file *stream, > - ui_file_fputs_ftype *fputs); > - > -/* This version of "write" is safe for use in signal handlers. > - It's not guaranteed that all existing output will have been > - flushed first. > - Implementations are also free to ignore some or all of the request. > - fputs_async is not provided as the async versions are rarely used, > - no point in having both for a rarely used interface. */ > -typedef void (ui_file_write_async_safe_ftype) > - (struct ui_file *stream, const char *buf, long length_buf); > -extern void set_ui_file_write_async_safe > - (struct ui_file *stream, ui_file_write_async_safe_ftype > *write_async_safe); > - > -typedef long (ui_file_read_ftype) (struct ui_file *stream, > - char *buf, long length_buf); > -extern void set_ui_file_read (struct ui_file *stream, > - ui_file_read_ftype *fread); > - > -typedef int (ui_file_isatty_ftype) (struct ui_file *stream); > -extern void set_ui_file_isatty (struct ui_file *stream, > - ui_file_isatty_ftype *isatty); > - > -typedef void (ui_file_rewind_ftype) (struct ui_file *stream); > -extern void set_ui_file_rewind (struct ui_file *stream, > - ui_file_rewind_ftype *rewind); > - > typedef void (ui_file_put_method_ftype) (void *object, const char > *buffer, > long length_buffer); This one can go too (actually you've marked it as deleted in the changelog). > -typedef void (ui_file_put_ftype) (struct ui_file *stream, > - ui_file_put_method_ftype *method, > - void *context); > -extern void set_ui_file_put (struct ui_file *stream, ui_file_put_ftype > *put); > > -typedef void (ui_file_delete_ftype) (struct ui_file * stream); > -extern void set_ui_file_data (struct ui_file *stream, void *data, > - ui_file_delete_ftype *to_delete); > +/* The ui_file base class. */ > > -typedef int (ui_file_fseek_ftype) (struct ui_file *stream, long > offset, > - int whence); > -extern void set_ui_file_fseek (struct ui_file *stream, > - ui_file_fseek_ftype *fseek_ptr); > +class ui_file > +{ > +public: > + ui_file (); > + virtual ~ui_file (); > > -extern void *ui_file_data (struct ui_file *file); > + /* Public non-virtual API. */ > > + void printf (const char *, ...) ATTRIBUTE_PRINTF (2, 3); > > -extern void gdb_flush (struct ui_file *); > + /* Print a string whose delimiter is QUOTER. Note that these > + routines should only be call for printing things which are call -> called? > + /* Rewind local buffer. I.e., clear it. */ > + virtual void rewind () > + {} I'm not sure about the implications, but instinctively I would've made rewind in the base class throw an exception by default, instead of making the derived types throw one if they don't support rewind. Actually, except for the null_file, I don't really see when you would want rewind to be a no-op. If the rewind call returns but does not actually rewind, isn't it lying to the client code? > > -extern void ui_file_delete (struct ui_file *stream); > + /* Write contents of local buffer onto WRITE. */ > + virtual void write_buffer_on (ui_file &where) Mismatch between parameter name and doc. > + /* Provide a few convenience methods with the same API as the > + underlying std::string. */ > + const char *data () const { return m_string.data (); } > + const char *c_str () const { return m_string.c_str (); } > + size_t size () const { return m_string.size (); } > + bool empty () const; I am wondering why you didn't implement empty inline like the others, since it's very similar. > + > +private: > + /* The internal buffer. */ > + std::string m_string; > +}; > + > +/* A ui_file implementation that maps directly onto 's > + FILE. */ > + > +class stdio_file : public ui_file > +{ > +public: > + /* Create a ui_file from a previously opened FILE. If not CLOSE_P, > + then behave like like fdopen(). */ "like like". It's also not very clear just saying "behave like fdopen()". Could it be a bit more explicit? > + explicit stdio_file (FILE *file, bool close_p = false); > + > + /* Create an stdio_file that is not managing any file yet. Call > + open to actually open something. */ > + stdio_file (); > + > + ~stdio_file () override; > + > + /* Open NAME in mode MODE. Returns true on success, false > + otherwise. Destroying the stdio_file closes the underlying FILE > + handle. */ > + bool open (const char *name, const char *mode); What happens if you try to reuse that object by calling open twice? Does it leak an open file? > +class tee_file : public ui_file > +{ > +public: > + /* Create a file which writes to both ONE and TWO. CLOSE_ONE and > + CLOSE_TWO indicate whether the original files should be closed > + when the new file is closed. */ > + tee_file (ui_file *one, bool close_one, > + ui_file *two, bool close_two); > + ~tee_file () override; > > -/* Open/create a STDIO based UI_FILE using the already open FILE. */ > -extern struct ui_file *stdio_fileopen (FILE *file); > + void write (const char *buf, long length_buf) override; > + void write_async_safe (const char *buf, long length_buf) override; > + void puts (const char *) override; > > -/* Likewise, for stderr-like streams. */ > -extern struct ui_file *stderr_fileopen (FILE *file); > + bool isatty () override; > + void flush () override; > > + void rewind () override > + { gdb_assert_not_reached ("tee_file::rewind called\n"); } > > -/* Open NAME returning an STDIO based UI_FILE. */ > -extern struct ui_file *gdb_fopen (const char *name, const char *mode); > +private: > + /* The two underlying ui_files, and whether they should each be > + closed on destruction. */ > + struct ui_file *m_one, *m_two; Nit: I'm a big fan of dropping the struct keyword. Thanks, Simon