From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11597 invoked by alias); 30 Nov 2016 22:38:06 -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 11584 invoked by uid 89); 30 Nov 2016 22:38:05 -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=hurry, mis 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; Wed, 30 Nov 2016 22:38:02 +0000 Received: by simark.ca (Postfix, from userid 33) id 1BE381E18F; Wed, 30 Nov 2016 17:38:01 -0500 (EST) To: Pedro Alves Subject: Re: [PATCH 19/22] Class-ify ui_out_impl 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: Wed, 30 Nov 2016 22:38:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: References: <20161124152428.24725-1-simon.marchi@polymtl.ca> <1480173587-7053-19-git-send-email-simon.marchi@polymtl.ca> Message-ID: <8ca5a7eb3f6adac16c8db785bd9fa6d3@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.2 X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg01031.txt.bz2 On 2016-11-30 08:09, Pedro Alves wrote: >> +ui_file * >> +cli_out_set_stream (struct ui_out *uiout, ui_file *stream) >> +{ >> + cli_ui_out_impl *impl = dynamic_cast >> (ui_out_impl (uiout)); > > "dynamic_cast" is a code smell. In this case, I think it > stems from the fact that we have separate ui_out and ui_out_impl > hierarchies. If we had a simple hierarchy, then this function would > take a cli_ui_out pointer as argument, or even be a > method of cli_ui_out. Indeed it's not great. This patchset is mostly about keeping the structure we have, but express it as C++. So I'd argue that it uncovers something that already existed. I think it makes the situation a tiny bit better though, as we have some type checking with dynamic_cast. With the static cast we have currently, if you happened to pass the wrong kind of object, it would probably fail catastrophically. I'll consider working on merging ui_out and ui_out_impl_* in a single class hierarchy. My first question is: what is a good pattern for the overlapping methods. For example, table_begin. We'll want to execute the version of the base class, which will then call the specialization. So we can't use the same name for both. So, keep table_begin for the base class, and table_begin_impl for the derived classes? >> +class cli_ui_out_impl : public ui_out_impl_base >> +{ >> +public: >> >> -struct cli_ui_out_data >> - { >> - std::vector streams; >> - int suppress_output; >> - }; >> + cli_ui_out_impl (ui_file *stream); > > explicit. Done. >> +private: >> + void field_separator (void); > > "(void)". Probably easier to grep the whole patch set to > find such cases and handle all of them. Yep, that's what I did the first time I saw that comment. >> +mi_ui_out_impl::table_begin (int nr_cols, int nr_rows, >> + const char *tblid) >> { >> - mi_open (uiout, tblid, ui_out_type_tuple); >> - mi_field_int (uiout, -1, -1, ui_left, "nr_rows", nr_rows); >> - mi_field_int (uiout, -1, -1, ui_left, "nr_cols", nr_cols); >> - mi_open (uiout, "hdr", ui_out_type_list); >> + open (tblid, ui_out_type_tuple); > > FYI, it's due to names like "open" potentially conflicting > with gnulib replacing global namespace C functions with > #define open rpl_open > that I'm looking at switching to use gnulib's C++ namespace > support. Luckily, looks like gnulib doesn't replace > "open" currently, so you don't need to wait for that: > > $ grep -rn open | grep rpl_ > stdio.in.h:191:# define fdopen rpl_fdopen > stdio.in.h:264:# define fopen rpl_fopen > stdio.in.h:388:# define freopen rpl_freopen > stdio.in.h:820:# define popen rpl_popen > dirent.in.h:79:# define opendir rpl_opendir > dirent.in.h:198:# define fdopendir rpl_fdopendir > >> void >> -mi_table_end (struct ui_out *uiout) >> +mi_ui_out_impl::table_end () >> { >> - mi_out_data *data = (mi_out_data *) ui_out_data (uiout); >> - >> - data->suppress_output = 0; >> - mi_close (uiout, ui_out_type_list); /* body */ >> - mi_close (uiout, ui_out_type_tuple); >> + m_suppress_output = 0; >> + close (ui_out_type_list); /* body */ >> + close (ui_out_type_tuple); > > However: > > $ grep -rn close | grep rpl_ > stdio.in.h:172:# define fclose rpl_fclose > dirent.in.h:131:# define closedir rpl_closedir > unistd.in.h:303:# define close rpl_close > > > :-/ > > I'd like to post the gnulib namespace patch this week, > but I'm not sure I'll be able to. And I guess it happens to work anyway for me because both the declaration, definition and usages get replaced? Should I wait for your patch to get in (I'm not particularly in a hurry), or we can get it in despite "close" getting replaced? >> + virtual void field_string (int fldno, int width, ui_align align, >> + const char *fldname, const char *string) override; >> + virtual void ATTRIBUTE_PRINTF(6,0) >> + field_fmt (int fldno, int width, ui_align align, const char >> *fldname, >> + const char *format, va_list args) override; > > It's more usual to put the ATTRIBUTE_PRINTFs at the end of the > declaration. Also, missing space before parens. Something like: > > virtual void field_fmt (int fldno, int width, ui_align align, > const char *fldname, > const char *format, va_list args) override > ATTRIBUTE_PRINTF (6,0); > > ? Ah, I put them there because I though it was the only place where it worked when applied to virtual pure functions. But apparently, this works: 98 virtual void field_fmt (int fldno, int width, ui_align align, 99 const char *fldname, const char *format, va_list args) 100 ATTRIBUTE_PRINTF(6,0) = 0; I thought I had tried it and it didn't work, but I was wrong. I'll fix them so they look like that. >> + virtual void spaces (int numspaces) override; >> + virtual void text (const char *string) override; >> + virtual void ATTRIBUTE_PRINTF(2,0) message (const char *format, >> va_list args) >> + override; > > Ditto. There are other instances in the patch. I won't point them > all. I'll go over all the patches and fix that. >> +private: >> + >> + void field_separator (); >> + void open (const char *name, ui_out_type type); >> + void close (ui_out_type type); >> + >> + int m_suppress_field_separator; >> + int m_suppress_output; > > bool ? Right. I changed it, but from what I can see MI's m_suppress_output is never used (as in never set to true). If that's indeed the case, I'll make a separate patch to remove it in the current master code. CLI has a m_suppress_output that is used though, I changed it as well to bool. >> + int m_mi_version; >> + std::vector m_streams; >> +}; > > > >> -typedef struct tui_ui_out_data tui_out_data; >> +class tui_ui_out_impl : public cli_ui_out_impl >> +{ >> +public: >> + >> + tui_ui_out_impl (ui_file *stream); > > explicit. Done. >> + >> + void field_int (int fldno, int width, ui_align align, const char >> *fldname, >> + int value) override; >> + void field_string (int fldno, int width, ui_align align, const char >> *fldname, >> + const char *string) override; >> + void ATTRIBUTE_PRINTF(6,0) >> + field_fmt (int fldno, int width, ui_align align, const char >> *fldname, >> + const char *format, va_list args) override; > > attribute placement, space before parens. Done. >> + void text (const char *string) override; >> > > >> + virtual int redirect (struct ui_file * outstream) = 0; >> + >> + /* Set as not MI-like by default. It is overridden in subclasses >> if >> + necessary. */ >> + >> + virtual int is_mi_like_p () >> + { return 0; } > > bool ? Done. Thanks for the review. I'll try to push the small patches that were OK'ed without comments, and re-post a complete v2 series.