diff mbox

[review] Add a string_view version of startswith

Message ID gerrit.1571229547000.I5389855de2fd70e7065a789a79374b0693651b71@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 16, 2019, 12:39 p.m. UTC 
  Christian Biesinger has uploaded a new change for review.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................

Add a string_view version of startswith

Makes sure that the string is longer than prefix, so that strncmp will
do the right thing even if the string is not null-terminated.

For use in my string_view conversion patch:
https://sourceware.org/ml/gdb-patches/2019-10/msg00030.html

gdb/ChangeLog:

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

	* gdbsupport/common-utils.h (startswith): Add an overloaded version
	that takes gdb::string_view arguments.

Change-Id: I5389855de2fd70e7065a789a79374b0693651b71
---
M gdb/gdbsupport/common-utils.h
1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Simon Marchi (Code Review) Oct. 22, 2019, 7:12 p.m. UTC | #1 
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 2:

(this has been reviewed at https://sourceware.org/ml/gdb-patches/2019-10/msg00035.html, but I'm waiting to land this until https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/125 is reviewed)
Simon Marchi (Code Review) Oct. 25, 2019, 5:35 p.m. UTC | #2 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 3: Code-Review+2

This was already approved elsewhere, but I'm marking it as such
in gerrit FAOD.
Simon Marchi (Code Review) Oct. 26, 2019, 11:10 p.m. UTC | #3 
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 3:

Today I was working in a completely unrelated area, and it turned out I needed
this function... I wonder what would happen if I submitted your patch to gerrit
as part of another series?

Maybe it's best not to find out.  Would you mind checking this one in?
Simon Marchi Oct. 27, 2019, 1:45 a.m. UTC | #4 
On 2019-10-26 7:10 p.m., Tom Tromey (Code Review) wrote:
> Tom Tromey has posted comments on this change.
> 
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
> ......................................................................
> 
> 
> Patch Set 3:
> 
> Today I was working in a completely unrelated area, and it turned out I needed
> this function... I wonder what would happen if I submitted your patch to gerrit
> as part of another series?
> 
> Maybe it's best not to find out.  Would you mind checking this one in?

If you base your patch on top of Christian's commit (d441ce34f4d2) and push for review,
nothing will happen to this patch.  Your new patch will just happen to have this one
as a parent.

If you rebase Christian's patch on master, add yours on top and push for review, then
it will add a new revision to Christian's patch (assuming the permissions allow it),
and create yours with this one as a parent.  And Christian will be grateful to you for
rebasing his patch :).

I think that you should try it.  For science.

> -- 
> Gerrit-Project: binutils-gdb
> Gerrit-Branch: master
> Gerrit-Change-Id: I5389855de2fd70e7065a789a79374b0693651b71
> Gerrit-Change-Number: 126
> Gerrit-PatchSet: 3
> Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
> Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
> Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
> Gerrit-Comment-Date: Sat, 26 Oct 2019 23:10:58 +0000
> Gerrit-HasComments: No
> Gerrit-Has-Labels: No
> Gerrit-MessageType: comment
>
Simon Marchi (Code Review) Oct. 27, 2019, 1:46 a.m. UTC | #5 
Simon Marchi (2) has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 3:

On 2019-10-26 7:10 p.m., Tom Tromey (Code Review) wrote:

If you base your patch on top of Christian's commit (d441ce34f4d2) and push for review,
nothing will happen to this patch.  Your new patch will just happen to have this one
as a parent.

If you rebase Christian's patch on master, add yours on top and push for review, then
it will add a new revision to Christian's patch (assuming the permissions allow it),
and create yours with this one as a parent.  And Christian will be grateful to you for
rebasing his patch :).

I think that you should try it.  For science.
Simon Marchi (Code Review) Oct. 27, 2019, 1:48 a.m. UTC | #6 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 3:

That part of my previous comment is misleading:

> On 2019-10-26 7:10 p.m., Tom Tromey (Code Review) wrote:

I replied by email, so that's the header my email client added when replying to Tom's comment, and Gerrit interpreted it as part of my comment.
Simon Marchi (Code Review) Oct. 27, 2019, 6:22 p.m. UTC | #7 
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 3:

> Patch Set 3:
> 
> If you base your patch on top of Christian's commit (d441ce34f4d2) and push for review,
> nothing will happen to this patch.  Your new patch will just happen to have this one
> as a parent.

Interestingly that was not *quite* my experience when I did something very similar:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
that should have tromey's https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176 as parent, but it doesn't show up that way in Gerrit. However the diff itself shows up correctly, relative to tromey's change.

> If you rebase Christian's patch on master, add yours on top and push for review, then
> it will add a new revision to Christian's patch (assuming the permissions allow it),
> and create yours with this one as a parent.  And Christian will be grateful to you for
> rebasing his patch :).
> 
> I think that you should try it.  For science.

I'm happy to push this, but I'll hold off for now in case you want to try this first. Let me know.
Simon Marchi (Code Review) Oct. 27, 2019, 10:10 p.m. UTC | #8 
Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 4:

FWIW, I now see Tom's patches in the relation chain on the right, on top of this patch.
Simon Marchi (Code Review) Oct. 28, 2019, 5:20 p.m. UTC | #9 
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/126
......................................................................


Patch Set 4:

> > If you rebase Christian's patch on master, add yours on top and push for review, then
> > it will add a new revision to Christian's patch (assuming the permissions allow it),
> > and create yours with this one as a parent.  And Christian will be grateful to you for
> > rebasing his patch :).
> > 
> > I think that you should try it.  For science.
> 
> I'm happy to push this, but I'll hold off for now in case you want to try this first. Let me know.

OK, now that we have the science results :) I am going to push this as requested.
diff mbox

Patch

diff --git a/gdb/gdbsupport/common-utils.h b/gdb/gdbsupport/common-utils.h
index a5312cb..e76b5b8 100644
--- a/gdb/gdbsupport/common-utils.h
+++ b/gdb/gdbsupport/common-utils.h
@@ -23,6 +23,7 @@ 
 #include <string>
 #include <vector>
 
+#include "gdb_string_view.h"
 #include "poison.h"
 
 /* If possible, define FUNCTION_NAME, a macro containing the name of
@@ -110,15 +111,25 @@ 
 
 extern char *safe_strerror (int);
 
-/* Return non-zero if the start of STRING matches PATTERN, zero
+/* Return true if the start of STRING matches PATTERN, false
    otherwise.  */
 
-static inline int
+static inline bool
 startswith (const char *string, const char *pattern)
 {
   return strncmp (string, pattern, strlen (pattern)) == 0;
 }
 
+/* Version of startswith that takes string_view arguments.  See comment
+   above.  */
+
+static inline bool
+startswith (gdb::string_view string, gdb::string_view pattern)
+{
+  return (string.length () >= pattern.length ()
+	  && strncmp (string.data (), pattern.data (), pattern.length ()) == 0);
+}
+
 ULONGEST strtoulst (const char *num, const char **trailer, int base);
 
 /* Skip leading whitespace characters in INP, returning an updated