Patchwork Add missing includes to gdb_assert.h and gdb_string_view.h

login
register
mail settings
Submitter Doug Evans via gdb-patches
Date Oct. 1, 2019, 6:37 p.m.
Message ID <20191001183730.214317-1-cbiesinger@google.com>
Download mbox | patch
Permalink /patch/34769/
State New
Headers show

Comments

Doug Evans via gdb-patches - Oct. 1, 2019, 6:37 p.m.
gdb::string_view uses gdb_assert, so it should include that header.
And gdb_assert uses internal_error, so it should include errors.h.

gdb/ChangeLog:

2019-10-01  Christian Biesinger  <cbiesinger@google.com>

	* gdbsupport/gdb_assert.h: Include errors.h.
	* gdbsupport/gdb_string_view.h: Include gdb_assert.h.
---
 gdb/gdbsupport/gdb_assert.h      | 2 ++
 gdb/gdbsupport/gdb_string_view.h | 1 +
 2 files changed, 3 insertions(+)
Simon Marchi - Oct. 2, 2019, 3:05 a.m.
On 2019-10-01 2:37 p.m., Christian Biesinger via gdb-patches wrote:
> gdb::string_view uses gdb_assert, so it should include that header.
> And gdb_assert uses internal_error, so it should include errors.h.

It might be that in the past, files like gdb_assert.h just assumed that
they were included as part of defs.h, and that necessary includes were
already included by defs.h.  Not sure.

In any case, I don't really see any downside going with the "include what
you use" route, so this patch would LGTM.  I'm just curious to see if others
have any objection to this, so please wait a bit before merging it.

Simon
Tom Tromey - Oct. 2, 2019, 3:56 p.m.
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> In any case, I don't really see any downside going with the "include what
Simon> you use" route, so this patch would LGTM.  I'm just curious to see if others
Simon> have any objection to this, so please wait a bit before merging it.

It seems fine to me.  A redundant include is pretty harmless overall; I
think the main include problem is a "new" include that isn't needed, as
these add to compilation time.

Tom
Doug Evans via gdb-patches - Oct. 2, 2019, 6:34 p.m.
On Wed, Oct 2, 2019 at 10:56 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>
> Simon> In any case, I don't really see any downside going with the "include what
> Simon> you use" route, so this patch would LGTM.  I'm just curious to see if others
> Simon> have any objection to this, so please wait a bit before merging it.
>
> It seems fine to me.  A redundant include is pretty harmless overall; I
> think the main include problem is a "new" include that isn't needed, as
> these add to compilation time.

Thanks, pushed.

I also found https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html
so including the same file twice should be even cheaper than I
thought.

Christian

Patch

diff --git a/gdb/gdbsupport/gdb_assert.h b/gdb/gdbsupport/gdb_assert.h
index a719d87899..6113050320 100644
--- a/gdb/gdbsupport/gdb_assert.h
+++ b/gdb/gdbsupport/gdb_assert.h
@@ -19,6 +19,8 @@ 
 #ifndef COMMON_GDB_ASSERT_H
 #define COMMON_GDB_ASSERT_H
 
+#include "errors.h"
+
 /* A static assertion.  This will cause a compile-time error if EXPR,
    which must be a compile-time constant, is false.  */
 
diff --git a/gdb/gdbsupport/gdb_string_view.h b/gdb/gdbsupport/gdb_string_view.h
index 68f7f7de36..19ae222527 100644
--- a/gdb/gdbsupport/gdb_string_view.h
+++ b/gdb/gdbsupport/gdb_string_view.h
@@ -46,6 +46,7 @@  namespace gdb {
 
 #include <string>
 #include <limits>
+#include "gdb_assert.h"
 
 namespace gdb {