develooper Front page | perl.perl5.porters | Postings from January 2017

[perl #130591] [PATCH] Make arguments to S_invlist_is_iterating andS_invlist_max be const

From:
Tony Cook via RT
Date:
January 30, 2017 22:52
Subject:
[perl #130591] [PATCH] Make arguments to S_invlist_is_iterating andS_invlist_max be const
Message ID:
rt-4.0.24-32456-1485816708-128.130591-15-0@perl.org
On Wed, 25 Jan 2017 21:03:33 -0800, khw wrote:
> On Sat, 21 Jan 2017 01:11:24 -0800, hv wrote:
> > On Fri, 20 Jan 2017 07:02:54 -0800, petdance wrote:
> > > > On Jan 20, 2017, at 3:29 AM, hv@crypt.org wrote:
> > > > Why the additional change here, was it somehow required for the
> > > > consting?
> > >
> > > Yes.
> >
> > Thanks, but I had hoped for a more engaged response.
> >
> > I had also written:
> > > It looks like care had been taken to abstract all references to
> > > iterator
> > > behind get_invlist_iter_addr(), this would violate that
> > > abstraction.
> >
> > 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?
> 
> I strongly prefer keeping the encapsulation.

I agree, I wouldn't apply this patch as is.

This case

 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;
 }

is one where I think you'd be justified (in C) in casting away const,
so something like:

+    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
used to modify the iter value.

In C++ you'd just write const and non-const get_invlist_iter_addr() accessors.

Tony

---
via perlbug:  queue: perl5 status: open
https://rt.perl.org/Ticket/Display.html?id=130591



nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About