From: Simon Marchi <simon.marchi@polymtl.ca>
To: Tom Tromey <tom@tromey.com>,
Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function
Date: Thu, 30 Sep 2021 12:17:47 -0400 [thread overview]
Message-ID: <509cd9b9-48d4-a9fe-31c9-05a8b66b4dee@polymtl.ca> (raw)
In-Reply-To: <87r1ff9anc.fsf@tromey.com>
On 2021-07-30 09:47, Tom Tromey wrote:
> Simon> The following patch wants to chagne gdb_fopen_cloexec to return a
>
> "change"
> Also I think that should be gdb_mkostemp_cloexec?
Both, actually.
>
> Simon> scoped_fd. Doing this causes a cyclic include between scoped_fd.h and
> Simon> filestuff.h. scoped_fd.h includes filestuff.h because of the
> Simon> scoped_fd::to_file method. To fix that, change scoped_fd::to_file to
> Simon> be a free function in filestuff.h.
>
> I guess that, while I don't really object, it seems like it would be
> better to break the dependency some other way. For instance would
> introducing a new header that only declares gdb_file_up work? The idea
> behind this is not to let the oddities of C++ header file management
> influence how the code itself is spelled -- that seems like a kind of
> inversion to me. Going along with this is the idea that a method is the
> clearest way to write to_file. These are all opinions of course, maybe
> there's a reason to prefer a free function.
There's three way to do this:
- scoped_fd::to_file method
- gdb_file::from_fd static method
- a free function
I never know which is best, which object should know about the other
one, or if no object should no about the other (and hence use a free
function).
But I agree with you here. See the updated 1/3 patch below.
Patch 2 changes trivially to update the includes and a to_file call.
From 7a281de194629ba00546c2d34cc41d2f77ccc5b3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Thu, 22 Jul 2021 11:34:57 -0400
Subject: [PATCH] gdbsupport: move gdb_file_up to its own file
The following patches wants to change gdb_fopen_cloexec and
gdb_mkostemp_cloexec to return a scoped_fd. Doing this causes a cyclic
include between scoped_fd.h and filestuff.h, that both want to include
each other. scoped_fd.h includes filestuff.h because of the
scoped_fd::to_file method's return value. filestuff.h would then
include scoped_fd.h for gdb_fopen_cloexec's and gdb_mkostemp_cloexec's
return values.
To fix that, move gdb_file_up to its own file, gdb_file.h.
Change-Id: Ic82a48914b2aacee8f14af535b7469245f88b93d
---
gdbsupport/filestuff.h | 13 +------------
gdbsupport/gdb_file.h | 37 +++++++++++++++++++++++++++++++++++++
gdbsupport/scoped_fd.h | 2 +-
3 files changed, 39 insertions(+), 13 deletions(-)
create mode 100644 gdbsupport/gdb_file.h
diff --git a/gdbsupport/filestuff.h b/gdbsupport/filestuff.h
index 7f4cfb2388b1..aa15f89aa920 100644
--- a/gdbsupport/filestuff.h
+++ b/gdbsupport/filestuff.h
@@ -21,6 +21,7 @@
#include <dirent.h>
#include <fcntl.h>
+#include "gdb_file.h"
/* Note all the file descriptors which are open when this is called.
These file descriptors will not be closed by close_most_fds. */
@@ -69,18 +70,6 @@ gdb_open_cloexec (const std::string &filename, int flags,
return gdb_open_cloexec (filename.c_str (), flags, mode);
}
-struct gdb_file_deleter
-{
- void operator() (FILE *file) const
- {
- fclose (file);
- }
-};
-
-/* A unique pointer to a FILE. */
-
-typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
-
/* Like 'fopen', but ensures that the returned file descriptor has the
close-on-exec flag set. */
diff --git a/gdbsupport/gdb_file.h b/gdbsupport/gdb_file.h
new file mode 100644
index 000000000000..2f5b399f6e87
--- /dev/null
+++ b/gdbsupport/gdb_file.h
@@ -0,0 +1,37 @@
+/* gdb_file_up, an RAII wrapper around FILE.
+ Copyright (C) 2021 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDBSUPPORT_GDB_FILE
+#define GDBSUPPORT_GDB_FILE
+
+#include <memory>
+#include <stdio.h>
+
+struct gdb_file_deleter
+{
+ void operator() (FILE *file) const
+ {
+ fclose (file);
+ }
+};
+
+/* A unique pointer to a FILE. */
+
+typedef std::unique_ptr<FILE, gdb_file_deleter> gdb_file_up;
+
+#endif
diff --git a/gdbsupport/scoped_fd.h b/gdbsupport/scoped_fd.h
index a1aad8493998..f16e811a2cef 100644
--- a/gdbsupport/scoped_fd.h
+++ b/gdbsupport/scoped_fd.h
@@ -21,7 +21,7 @@
#define COMMON_SCOPED_FD_H
#include <unistd.h>
-#include "filestuff.h"
+#include "gdb_file.h"
/* A smart-pointer-like class to automatically close a file descriptor. */
--
2.33.0
next prev parent reply other threads:[~2021-09-30 16:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-22 19:37 Simon Marchi
2021-07-22 19:37 ` [PATCH 2/3] gdbsupport: make gdb_open_cloexec return scoped_fd Simon Marchi
2021-07-22 19:37 ` [PATCH 3/3] gdbsupport: make gdb_mkostemp_cloexec return a scoped_fd Simon Marchi
2021-07-30 13:47 ` [PATCH 1/3] gdbsupport: change scoped_fd::to_file to a free function Tom Tromey
2021-09-30 16:17 ` Simon Marchi [this message]
2021-09-30 17:48 ` Tom Tromey
2021-09-30 19:29 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=509cd9b9-48d4-a9fe-31c9-05a8b66b4dee@polymtl.ca \
--to=simon.marchi@polymtl.ca \
--cc=gdb-patches@sourceware.org \
--cc=tom@tromey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).