strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Mark Roberts
When the BCEL package was renamed back to org.apache.bcel  in:

commit d522432b79044740831a132d8b92e7dab5477444
Author: Benedikt Ritter <[hidden email]>
Date:   Tue Jun 7 17:28:43 2016 +0000

The methods to add and delete annotations were changed from public to protected with a confusing comment:
<     public void addAnnotationEntry(AnnotationEntryGen ag)
---
>     protected void addAnnotationEntry(final AnnotationEntryGen ag) // TODO could this be package protected?

I think this might have been a cut and paste error as the same comment was added to other methods, but they were left public (so the comment makes sense).

In any case, the current situation is you can add and delete Attributes but not Annotations.  And, surprise, that is exactly what I need to do.

Any reason not to change these back to public?

Thanks,
Mark



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

sebb-2-2
On Thu, 17 Oct 2019 at 23:16, Mark Roberts <[hidden email]> wrote:

>
> When the BCEL package was renamed back to org.apache.bcel  in:
>
> commit d522432b79044740831a132d8b92e7dab5477444
> Author: Benedikt Ritter <[hidden email]>
> Date:   Tue Jun 7 17:28:43 2016 +0000
>
> The methods to add and delete annotations were changed from public to protected with a confusing comment:
> <     public void addAnnotationEntry(AnnotationEntryGen ag)
> ---
> >     protected void addAnnotationEntry(final AnnotationEntryGen ag) // TODO could this be package protected?

The comment makes sense to me.

The method was changed from public to protected, reducing the visibility.
The TODO asks if it could be made package protected, i.e. further
reducing the visibility.

> I think this might have been a cut and paste error as the same comment was added to other methods, but they were left public (so the comment makes sense).

It seems to me that the other methods were also probably intended to
be reduced in visibility to protected, but this was not done.

> In any case, the current situation is you can add and delete Attributes but not Annotations.  And, surprise, that is exactly what I need to do.
>
> Any reason not to change these back to public?

Increasing visibility increases the difficulty of testing and
increases the likelihood of subtle bugs.

> Thanks,
> Mark
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

RE: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Mark Roberts
So I'm forced to add pass through methods to MethodGen?  That seems a waste of effort and still requires testing.  I repeat - you can already manipulate Attrbiutes - you should be able to manipulate Annotations in exactly the same fashion.  It is a missing capability that is needed - at least by me and as we move forward into Java 9 and beyond I'm sure others are going to run the problem.

-----Original Message-----
From: sebb [mailto:[hidden email]]
Sent: Thursday, October 17, 2019 3:55 PM
To: Commons Developers List <[hidden email]>
Subject: Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

On Thu, 17 Oct 2019 at 23:16, Mark Roberts <[hidden email]> wrote:

>
> When the BCEL package was renamed back to org.apache.bcel  in:
>
> commit d522432b79044740831a132d8b92e7dab5477444
> Author: Benedikt Ritter <[hidden email]>
> Date:   Tue Jun 7 17:28:43 2016 +0000
>
> The methods to add and delete annotations were changed from public to protected with a confusing comment:
> <     public void addAnnotationEntry(AnnotationEntryGen ag)
> ---
> >     protected void addAnnotationEntry(final AnnotationEntryGen ag) // TODO could this be package protected?

The comment makes sense to me.

The method was changed from public to protected, reducing the visibility.
The TODO asks if it could be made package protected, i.e. further reducing the visibility.

> I think this might have been a cut and paste error as the same comment was added to other methods, but they were left public (so the comment makes sense).

It seems to me that the other methods were also probably intended to be reduced in visibility to protected, but this was not done.

> In any case, the current situation is you can add and delete Attributes but not Annotations.  And, surprise, that is exactly what I need to do.
>
> Any reason not to change these back to public?

Increasing visibility increases the difficulty of testing and increases the likelihood of subtle bugs.

> Thanks,
> Mark
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Claude Warren
The change from public to private would indicate a major version change as
it changes the API.  Though I suppose this could also be done if code were
being contributed to a project from outside.  In which case the minor
(middle) number would have to have changed.

In either case changing from a protected to a public method would be
allowed under semantic numbering as a patch change.

As this is a patch level change, and there is a need for the functionality
perhaps it should revert to the public form that it had in the past.

@sebbaz can you explain why making a method public makes it more difficult
to test?  Seems counter intuitive to me.

Claude



On Fri, Oct 18, 2019 at 1:55 AM Mark Roberts <[hidden email]>
wrote:

> So I'm forced to add pass through methods to MethodGen?  That seems a
> waste of effort and still requires testing.  I repeat - you can already
> manipulate Attrbiutes - you should be able to manipulate Annotations in
> exactly the same fashion.  It is a missing capability that is needed - at
> least by me and as we move forward into Java 9 and beyond I'm sure others
> are going to run the problem.
>
> -----Original Message-----
> From: sebb [mailto:[hidden email]]
> Sent: Thursday, October 17, 2019 3:55 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: strange change to
> src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java
>
> On Thu, 17 Oct 2019 at 23:16, Mark Roberts <[hidden email]>
> wrote:
> >
> > When the BCEL package was renamed back to org.apache.bcel  in:
> >
> > commit d522432b79044740831a132d8b92e7dab5477444
> > Author: Benedikt Ritter <[hidden email]>
> > Date:   Tue Jun 7 17:28:43 2016 +0000
> >
> > The methods to add and delete annotations were changed from public to
> protected with a confusing comment:
> > <     public void addAnnotationEntry(AnnotationEntryGen ag)
> > ---
> > >     protected void addAnnotationEntry(final AnnotationEntryGen ag) //
> TODO could this be package protected?
>
> The comment makes sense to me.
>
> The method was changed from public to protected, reducing the visibility.
> The TODO asks if it could be made package protected, i.e. further reducing
> the visibility.
>
> > I think this might have been a cut and paste error as the same comment
> was added to other methods, but they were left public (so the comment makes
> sense).
>
> It seems to me that the other methods were also probably intended to be
> reduced in visibility to protected, but this was not done.
>
> > In any case, the current situation is you can add and delete Attributes
> but not Annotations.  And, surprise, that is exactly what I need to do.
> >
> > Any reason not to change these back to public?
>
> Increasing visibility increases the difficulty of testing and increases
> the likelihood of subtle bugs.
>
> > Thanks,
> > Mark
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

--
I like: Like Like - The likeliest place on the web
<http://like-like.xenei.com>
LinkedIn: http://www.linkedin.com/in/claudewarren
Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

sebb-2-2
On Fri, 18 Oct 2019 at 08:06, Claude Warren <[hidden email]> wrote:
>
> The change from public to private would indicate a major version change as
> it changes the API.  Though I suppose this could also be done if code were
> being contributed to a project from outside.  In which case the minor
> (middle) number would have to have changed.

As noted in the OP, the change was part of changing the package name.

> In either case changing from a protected to a public method would be
> allowed under semantic numbering as a patch change.
>
> As this is a patch level change, and there is a need for the functionality
> perhaps it should revert to the public form that it had in the past.

Not relevant here.

> @sebbaz can you explain why making a method public makes it more difficult
> to test?  Seems counter intuitive to me.

I meant that it is harder to test for all possible scenarios, because
there are more opportunities to invoke the method.

More restricted methods have a smaller context.
In particular, package-protected and private methods have almost total
control over how they are invoked.

Otherwise all methods might as well be public.

> Claude
>
>
>
> On Fri, Oct 18, 2019 at 1:55 AM Mark Roberts <[hidden email]>
> wrote:
>
> > So I'm forced to add pass through methods to MethodGen?  That seems a
> > waste of effort and still requires testing.  I repeat - you can already
> > manipulate Attrbiutes - you should be able to manipulate Annotations in
> > exactly the same fashion.  It is a missing capability that is needed - at
> > least by me and as we move forward into Java 9 and beyond I'm sure others
> > are going to run the problem.
> >
> > -----Original Message-----
> > From: sebb [mailto:[hidden email]]
> > Sent: Thursday, October 17, 2019 3:55 PM
> > To: Commons Developers List <[hidden email]>
> > Subject: Re: strange change to
> > src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java
> >
> > On Thu, 17 Oct 2019 at 23:16, Mark Roberts <[hidden email]>
> > wrote:
> > >
> > > When the BCEL package was renamed back to org.apache.bcel  in:
> > >
> > > commit d522432b79044740831a132d8b92e7dab5477444
> > > Author: Benedikt Ritter <[hidden email]>
> > > Date:   Tue Jun 7 17:28:43 2016 +0000
> > >
> > > The methods to add and delete annotations were changed from public to
> > protected with a confusing comment:
> > > <     public void addAnnotationEntry(AnnotationEntryGen ag)
> > > ---
> > > >     protected void addAnnotationEntry(final AnnotationEntryGen ag) //
> > TODO could this be package protected?
> >
> > The comment makes sense to me.
> >
> > The method was changed from public to protected, reducing the visibility.
> > The TODO asks if it could be made package protected, i.e. further reducing
> > the visibility.
> >
> > > I think this might have been a cut and paste error as the same comment
> > was added to other methods, but they were left public (so the comment makes
> > sense).
> >
> > It seems to me that the other methods were also probably intended to be
> > reduced in visibility to protected, but this was not done.
> >
> > > In any case, the current situation is you can add and delete Attributes
> > but not Annotations.  And, surprise, that is exactly what I need to do.
> > >
> > > Any reason not to change these back to public?
> >
> > Increasing visibility increases the difficulty of testing and increases
> > the likelihood of subtle bugs.
> >
> > > Thanks,
> > > Mark
> > >
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
> --
> I like: Like Like - The likeliest place on the web
> <http://like-like.xenei.com>
> LinkedIn: http://www.linkedin.com/in/claudewarren

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

sebb-2-2
In reply to this post by Mark Roberts
On Fri, 18 Oct 2019 at 01:55, Mark Roberts <[hidden email]> wrote:
>
> So I'm forced to add pass through methods to MethodGen?  That seems a waste of effort and still requires testing.  I repeat - you can already manipulate Attrbiutes - you should be able to manipulate Annotations in exactly the same fashion.  It is a missing capability that is needed - at least by me and as we move forward into Java 9 and beyond I'm sure others are going to run the problem.

I was responding to your statement about the comment.
I have not looked into whether that change was necessary or not.

If there are use-cases that are not covered by the current tests, it
would be very useful to get those added.

> -----Original Message-----
> From: sebb [mailto:[hidden email]]
> Sent: Thursday, October 17, 2019 3:55 PM
> To: Commons Developers List <[hidden email]>
> Subject: Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java
>
> On Thu, 17 Oct 2019 at 23:16, Mark Roberts <[hidden email]> wrote:
> >
> > When the BCEL package was renamed back to org.apache.bcel  in:
> >
> > commit d522432b79044740831a132d8b92e7dab5477444
> > Author: Benedikt Ritter <[hidden email]>
> > Date:   Tue Jun 7 17:28:43 2016 +0000
> >
> > The methods to add and delete annotations were changed from public to protected with a confusing comment:
> > <     public void addAnnotationEntry(AnnotationEntryGen ag)
> > ---
> > >     protected void addAnnotationEntry(final AnnotationEntryGen ag) // TODO could this be package protected?
>
> The comment makes sense to me.
>
> The method was changed from public to protected, reducing the visibility.
> The TODO asks if it could be made package protected, i.e. further reducing the visibility.
>
> > I think this might have been a cut and paste error as the same comment was added to other methods, but they were left public (so the comment makes sense).
>
> It seems to me that the other methods were also probably intended to be reduced in visibility to protected, but this was not done.
>
> > In any case, the current situation is you can add and delete Attributes but not Annotations.  And, surprise, that is exactly what I need to do.
> >
> > Any reason not to change these back to public?
>
> Increasing visibility increases the difficulty of testing and increases the likelihood of subtle bugs.
>
> > Thanks,
> > Mark
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Emmanuel Bourg-3
In reply to this post by sebb-2-2
Le 18/10/2019 à 10:52, sebb a écrit :

> As noted in the OP, the change was part of changing the package name.

If the visibility change triggered a regression I think it should be
reverted.

Emmanuel bourg

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Xeno Amess
Do commons follow semver?

Emmanuel Bourg <[hidden email]> 于 2019年10月18日周五 下午5:05写道:

> Le 18/10/2019 à 10:52, sebb a écrit :
>
> > As noted in the OP, the change was part of changing the package name.
>
> If the visibility change triggered a regression I think it should be
> reverted.
>
> Emmanuel bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Emmanuel Bourg-3
Le 18/10/2019 à 11:10, Xeno Amess a écrit :
> Do commons follow semver?

No but we care about backward compatibility.

Emmanuel Bourg

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

garydgregory
Hi All:

The upshot here (to me) is that we have an opportunity to help Mark by:

- Simply agreeing that it is OK to change the visibility of the method(s)
if appropriate tests are added, or:
- Provide a new API _somewhere_, presumably in the _best/right_ place that
fulfills his app's needs.

The difference being changing visibility solely for expediency's sake vs.
thinking about the API and making sure provide this functionality in the
right place.

It seems that Mark can help us see it best since he's quite familiar with
the code by now.

Gary

On Fri, Oct 18, 2019 at 5:37 AM Emmanuel Bourg <[hidden email]> wrote:

> Le 18/10/2019 à 11:10, Xeno Amess a écrit :
> > Do commons follow semver?
>
> No but we care about backward compatibility.
>
> Emmanuel Bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

RE: strange change to src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Mark Roberts
I really would like to move forward with this.  If you look at the code in
src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java you will see:

Public addAttribute ...
Protected addAnnotationEntry ...
Public removeAttribute ...
Protected removeAnnotationEntry ...
Public removeAttributes ...
Protected remove AnnotationEntries ...

I would just like to change the protected to public so one can manipulate
annotations in the same manner as attributes.

Thank you,
Mark

-----Original Message-----
From: Gary Gregory [mailto:[hidden email]]
Sent: Friday, October 18, 2019 8:29 AM
To: Commons Developers List
Subject: Re: strange change to
src/main/java/org/apache/bcel/generic/FieldGenOrMethodGen.java

Hi All:

The upshot here (to me) is that we have an opportunity to help Mark by:

- Simply agreeing that it is OK to change the visibility of the method(s) if
appropriate tests are added, or:
- Provide a new API _somewhere_, presumably in the _best/right_ place that
fulfills his app's needs.

The difference being changing visibility solely for expediency's sake vs.
thinking about the API and making sure provide this functionality in the
right place.

It seems that Mark can help us see it best since he's quite familiar with
the code by now.

Gary

On Fri, Oct 18, 2019 at 5:37 AM Emmanuel Bourg <[hidden email]> wrote:

> Le 18/10/2019 à 11:10, Xeno Amess a écrit :
> > Do commons follow semver?
>
> No but we care about backward compatibility.
>
> Emmanuel Bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]