New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PATCH] Make arguments to S_invlist_is_iterating and S_invlist_max be const #15815
Comments
From @petdanceCreated by @petdanceMake arguments to S_invlist_is_iterating and S_invlist_max be const. Perl Info
|
From @petdance0001-Make-arguments-to-S_invlist_is_iterating-and-S_invli.patchFrom a1c41da9a78a9cff927ec89161d6e81658d6e74d Mon Sep 17 00:00:00 2001
From: Andy Lester <andy@petdance.com>
Date: Thu, 19 Jan 2017 22:52:15 -0600
Subject: [PATCH] Make arguments to S_invlist_is_iterating and S_invlist_max be
const
---
embed.fnc | 4 ++--
proto.h | 4 ++--
regcomp.c | 6 +++---
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/embed.fnc b/embed.fnc
index 656afe5..af5e7fb 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -1635,10 +1635,10 @@ EXpM |UV |swash_fetch |NN SV *swash|NN const U8 *ptr|bool do_utf8
#ifdef PERL_IN_REGCOMP_C
EiMR |SV* |add_cp_to_invlist |NULLOK SV* invlist|const UV cp
EiM |void |invlist_set_len|NN SV* const invlist|const UV len|const bool offset
-EiMRn |bool |invlist_is_iterating|NN SV* const invlist
+EiMRn |bool |invlist_is_iterating|NN const SV* const invlist
#ifndef PERL_EXT_RE_BUILD
EiMRn |UV* |_invlist_array_init |NN SV* const invlist|const bool will_have_0
-EiMRn |UV |invlist_max |NN SV* const invlist
+EiMRn |UV |invlist_max |NN const SV* const invlist
EsM |void |_append_range_to_invlist |NN SV* const invlist|const UV start|const UV end
EsM |void |invlist_extend |NN SV* const invlist|const UV len
EsM |void |invlist_replace_list_destroys_src|NN SV *dest|NN SV *src
diff --git a/proto.h b/proto.h
index 2fd8a51..62526cb 100644
--- a/proto.h
+++ b/proto.h
@@ -3840,7 +3840,7 @@ PERL_STATIC_INLINE void S_invlist_clear(pTHX_ SV* invlist);
STATIC void S_invlist_extend(pTHX_ SV* const invlist, const UV len);
#define PERL_ARGS_ASSERT_INVLIST_EXTEND \
assert(invlist)
-PERL_STATIC_INLINE UV S_invlist_max(SV* const invlist)
+PERL_STATIC_INLINE UV S_invlist_max(const SV* const invlist)
__attribute__warn_unused_result__;
#define PERL_ARGS_ASSERT_INVLIST_MAX \
assert(invlist)
@@ -4966,7 +4966,7 @@ PERL_STATIC_INLINE UV S_invlist_highest(SV* const invlist)
#define PERL_ARGS_ASSERT_INVLIST_HIGHEST \
assert(invlist)
-PERL_STATIC_INLINE bool S_invlist_is_iterating(SV* const invlist)
+PERL_STATIC_INLINE bool S_invlist_is_iterating(const SV* const invlist)
__attribute__warn_unused_result__;
#define PERL_ARGS_ASSERT_INVLIST_IS_ITERATING \
assert(invlist)
diff --git a/regcomp.c b/regcomp.c
index 97888ca..a5feadc 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -8579,17 +8579,17 @@ S_invlist_clear(pTHX_ SV* invlist) /* Empty the inversion list */
#endif /* ifndef PERL_IN_XSUB_RE */
PERL_STATIC_INLINE bool
-S_invlist_is_iterating(SV* const invlist)
+S_invlist_is_iterating(const SV* const invlist)
{
PERL_ARGS_ASSERT_INVLIST_IS_ITERATING;
- return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX;
+ return ((XINVLIST*) SvANY(invlist))->iterator < (STRLEN)UV_MAX;
}
#ifndef PERL_IN_XSUB_RE
PERL_STATIC_INLINE UV
-S_invlist_max(SV* const invlist)
+S_invlist_max(const SV* const invlist)
{
/* Returns the maximum number of elements storable in the inversion list's
* array, without having to realloc() */
--
1.8.3.1
|
From @hvdsAndy Lester (via RT) <perlbug-followup@perl.org> wrote: Why the additional change here, was it somehow required for the consting? Hugo |
The RT System itself - Status changed from 'new' to 'open' |
From @petdance
Yes. -- |
From @hvdsOn Fri, 20 Jan 2017 07:02:54 -0800, petdance wrote:
Thanks, but I had hoped for a more engaged response. I had also written:
I understand and value the benefits of abstraction and encapsulation in a complex codebase, and find them all too rarely in perl's. I'm much vaguer on the benefits of consting - they seem pretty minor, and regularly outweighed by bugs introduced - so my default preference would be to retain the former and reject this patch. However it might also be possible to provide the benefits of both, eg by splitting the r/w accessor into separate rvalue and lvalue variants. Karl, I think you're the one most involved with this code, do you have any opinion? Hugo |
From @petdance
I'm sorry. I'd answered your question and didn't have anything else to add to your obvservation about abstractions.
I'm curious what bugs have been introduced by consting. I haven't seen any in the decade since I started working on using const in the Perl codebase. I'm curious what's gone wrong with it. I work hard to get the compiler and other tools do as much keeping an eye on us fallible humans as possible. The big point of consting function pointer parameters is to let the compiler know what goes on inside a function. Consider this code: char p[100]; You know and I know that that's bad, because p has not been initialized. The compiler will throw a warning that p is being used without being initialized. The compiler only knows that because strlen is defined strlen(const char *p). If not for that const on the char *p, the compiler has to assume that it's possible that strlen() is modifying the contents of p. So we need to have as many of our functions declared in the same way so the compiler can do its job better. So if S_invlist_max is not declared with a const pointer argument, the compiler may think that S_invlist_max is actually writing to the pointer contents. The other point of consting, in consting variables, is that it gives the reader a clue as to the intent of a variable. const int len = this + that + other; The first tells the reader that you're calculating this once, at the top of the block, and you're not going to modify it later on. In the second, the reader is now notified that this len is going to change later on in the block. And, if you do modify the len and get a compiler warning, the person making the change now needs to wonder "Should I really be modifying this value if that's not it's intent?"
I think that's an excellent idea. Two thumbs up from me. Thanks for pursuing it. -- |
From @craigberryOn Sat, Jan 21, 2017 at 10:15 PM, Andy Lester <andy@petdance.com> wrote:
I've seen estimates of something like 8-10 bugs introduced into the <https://rt-archive.perl.org/perl5/Ticket/Display.html?id=72426> which was a segfault cause by: <https://perl5.git.perl.org/perl.git/commitdiff/890ce7af62ab97fd07b5b49562f13e94286469fb> So there was an easy segfault (C<$ perl -etell>) in the wild for 5 |
From @petdance
You say "sometimes by the refactoring" but I suspect that they've all been because of refactoring, and not because of the consting.
Interesting, thanks for digging it up. That patch from 2005 is way too big, and I should have been broken it up into smaller more discrete patches. I haven't dug into it, but my guess is that it's the NN/NULLOK getting added to the parameter pointers that probably caused the problems. Jarkko pointed out a while back the problems with the notnull attributes and how they change the code generated, and we've since eliminated those from the codebase. -- |
From @cpansproutOn Sun, 22 Jan 2017 10:48:27 -0800, petdance wrote:
92adfbd is an example of refactoring that was solely for the sake of consting (according to <20060308064529.GA16499@petdance.com>). It caused bug #48332. -- Father Chrysostomos |
From @khwilliamsonOn Sat, 21 Jan 2017 01:11:24 -0800, hv wrote:
I strongly prefer keeping the encapsulation. As many others have said (I'm guessing), I wish this had been written in C++, and then people wouldn't even be tempted to break it. The underlying implementation of inversion lists was changed once already, and the effects were limited because of the encapsulation. We may change it again, so that it isn't an SV type, and this would break at that point. My position on consting is that I try to use it, but if it leads to any compile issues, I immediately abandon it.
-- |
From @demerphqOn 26 January 2017 at 06:03, Karl Williamson via RT
Me too. All too often it seems to be that adding a const attribute to Yves -- |
From @khwilliamsonOn 01/27/2017 10:39 AM, demerphq wrote:
I've thought if things had been designed properly to begin with that Also, note that const was not in the original C language. I programmed |
From @petdance
I’ve gone down that road before, at least twice, over the years. There’s a ton we could do, but as you suggest, it would have to be at the lowest level and worked upward. I always wound up abandoning my attempt branches. -- |
From @tonycozOn Wed, 25 Jan 2017 21:03:33 -0800, khw wrote:
I agree, I wouldn't apply this patch as is. This case PERL_STATIC_INLINE bool - return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX; is one where I think you'd be justified (in C) in casting away const, + return *(get_invlist_iter_addr((SV*)invlist)) < (STRLEN) UV_MAX; since invlist_is_iterating() doesn't modify the iter value. get_invlist_iter_addr() itself shouldn't be const, since its return value can be In C++ you'd just write const and non-const get_invlist_iter_addr() accessors. Tony |
From @tonycozOn Mon, 30 Jan 2017 14:51:48 -0800, tonyc wrote:
Like the attached. Done as a new patch due to merge conflicts against blead with the old one. Tony |
From @tonycoz0001-Make-arguments-to-S_invlist_is_iterating-and-S_invli.patchFrom daa1383eb2b9f69656e5bc0f25222be52ca6f5b1 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 22 May 2017 15:57:07 +1000
Subject: Make arguments to S_invlist_is_iterating and S_invlist_max be const
Based on a patch originally by Andy Lester
---
embed.fnc | 4 ++--
proto.h | 4 ++--
regcomp.c | 9 ++++++---
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/embed.fnc b/embed.fnc
index 654dad9..442f4b2 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -1631,10 +1631,10 @@ EXpM |UV |swash_fetch |NN SV *swash|NN const U8 *ptr|bool do_utf8
#ifdef PERL_IN_REGCOMP_C
EiMR |SV* |add_cp_to_invlist |NULLOK SV* invlist|const UV cp
EiM |void |invlist_set_len|NN SV* const invlist|const UV len|const bool offset
-EiMRn |bool |invlist_is_iterating|NN SV* const invlist
+EiMRn |bool |invlist_is_iterating|NN const SV* const invlist
#ifndef PERL_EXT_RE_BUILD
EiMRn |UV* |_invlist_array_init |NN SV* const invlist|const bool will_have_0
-EiMRn |UV |invlist_max |NN SV* const invlist
+EiMRn |UV |invlist_max |NN const SV* const invlist
EsM |void |_append_range_to_invlist |NN SV* const invlist|const UV start|const UV end
EsM |void |invlist_extend |NN SV* const invlist|const UV len
EsM |void |invlist_replace_list_destroys_src|NN SV *dest|NN SV *src
diff --git a/proto.h b/proto.h
index f1d6181..e5f340b 100644
--- a/proto.h
+++ b/proto.h
@@ -3917,7 +3917,7 @@ STATIC void S_invlist_extend(pTHX_ SV* const invlist, const UV len);
#define PERL_ARGS_ASSERT_INVLIST_EXTEND \
assert(invlist)
#ifndef PERL_NO_INLINE_FUNCTIONS
-PERL_STATIC_INLINE UV S_invlist_max(SV* const invlist)
+PERL_STATIC_INLINE UV S_invlist_max(const SV* const invlist)
__attribute__warn_unused_result__;
#define PERL_ARGS_ASSERT_INVLIST_MAX \
assert(invlist)
@@ -5103,7 +5103,7 @@ PERL_STATIC_INLINE UV S_invlist_highest(SV* const invlist)
#endif
#ifndef PERL_NO_INLINE_FUNCTIONS
-PERL_STATIC_INLINE bool S_invlist_is_iterating(SV* const invlist)
+PERL_STATIC_INLINE bool S_invlist_is_iterating(const SV* const invlist)
__attribute__warn_unused_result__;
#define PERL_ARGS_ASSERT_INVLIST_IS_ITERATING \
assert(invlist)
diff --git a/regcomp.c b/regcomp.c
index 54d641d..ab47a73 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -8606,17 +8606,20 @@ S_invlist_clear(pTHX_ SV* invlist) /* Empty the inversion list */
#endif /* ifndef PERL_IN_XSUB_RE */
PERL_STATIC_INLINE bool
-S_invlist_is_iterating(SV* const invlist)
+S_invlist_is_iterating(const SV* const invlist)
{
PERL_ARGS_ASSERT_INVLIST_IS_ITERATING;
- return *(get_invlist_iter_addr(invlist)) < (STRLEN) UV_MAX;
+ /* get_invlist_iter_addr()'s sv is non-const only because it returns a
+ * value that can be used to modify the invlist, it doesn't modify the
+ * invlist itself */
+ return *(get_invlist_iter_addr((SV*)invlist)) < (STRLEN) UV_MAX;
}
#ifndef PERL_IN_XSUB_RE
PERL_STATIC_INLINE UV
-S_invlist_max(SV* const invlist)
+S_invlist_max(const SV* const invlist)
{
/* Returns the maximum number of elements storable in the inversion list's
* array, without having to realloc() */
--
2.1.4
|
Migrated from rt.perl.org#130591 (status was 'open')
Searchable as RT130591$
The text was updated successfully, but these errors were encountered: