You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tim Ellison <t....@gmail.com> on 2006/04/20 15:12:00 UTC
[classlib] String is special
You'll recall a while ago when we were discussing moving j.l.String out
from KERNEL to LUNI [1] that the shape of String is something we would
expect VMs & JITs to be sensitive to (like all our KERNEL classes), but
that there is significant shared behavior that it is worth sharing
(which is why we moved it into LUNI).
I suggested that we could deal with this by ensuring changes to String
were closely monitored, and any such changes agreed on the list first
(thereby acquiring a 'golden ticket'). There have been a few changes to
String recently (I have reviewed them, and they look benign) but I'd
like to reiterate that call.
To ensure that all committers can continue to update String, but that
they do so 'knowingly' (i.e. after considering the consequences) I'd
like to impose a 'positive action' pre-commit step by setting the
"svn:needs-lock" property on String.java.
That will ensure that committers do not inadvertently (despite their
thorough diff review) apply a patch that modifies String.java. The
extra step required would simply be to acquire a lock on String.java
first and that should be enough to remind people that they are modifying
this class.
(This is for my benefit as much as anyone else's)
If there are no objections I'll go ahead and do this.
Regards,
Tim
[1]
http://mail-archives.apache.org/mod_mbox/incubator-harmony-dev/200602.mbox/%3C43FDB1AE.7060807@gmail.com%3E
--
Tim Ellison (t.p.ellison@gmail.com)
IBM Java technology centre, UK.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Stefano Mazzocchi <st...@apache.org>.
Etienne Gagnon wrote:
> Hi Tim,
>
> I have an objection.
>
> Locking has nothing to do with "commit control". Allowing for svn
> locking functionality is opening a can of worms. What if somebody
> aquires a lock and loses network connectivity for a week (because of a
> Hurricane, because he forgot and went on vacation, etc.)? The whole svn
> philosophy is to allow for parrallel development, instead of the
> serialized development imposed by locking based repositories (Visual
> Source Safe, RCS, etc).
>
> I would sugget, instead, to put a BIG WARNING at the top of String.java,
> indicating that any change must first be approved on harmony-dev@. I
> think that this would accomplish your goal in a more appropriate manner.
>
> Etienne
>
> Tim Ellison wrote:
>> To ensure that all committers can continue to update String, but that
>> they do so 'knowingly' (i.e. after considering the consequences) I'd
>> like to impose a 'positive action' pre-commit step by setting the
>> "svn:needs-lock" property on String.java.
>> ...
>> If there are no objections I'll go ahead and do this.
I'm with Etienne here, *big* -1 on locking.
--
Stefano.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Geir Magnusson Jr <ge...@pobox.com>.
So howabout a firmer test that will break when string is changed? that
might at least let you sleep better.
geir
Tim Ellison wrote:
> Ok, so reading everyone's post it seems that:
>
> - we agree that there should be some way to ensure people do
> not inadvertently modify String's shape, but
>
> - there are different ways to achieve that goal, and requiring
> people to acquire a lock is not favored.
>
> I propose that we leave this thread as a further reminder to all, and if
> we catch any transgressions that we revisit the mechanism.
>
> Just to reiterate, it is certainly not my intention to prevent people
> from working on String, I just want them to do so knowingly.
>
> Regards,
> Tim
>
> Etienne Gagnon wrote:
>> Geir Magnusson Jr wrote:
>>> 3) Lock down function and behavior tightly with tests - that if
>>> modified, tests will break, and that should raise an alarm to the
>>> committer.
>> One easy way would be to add a test that compares the String.java source
>> code with a saved version, and that fails if unequal. This way, any
>> change will trigger a test failure. To get past this failure, one will
>> have to change both String.java and the saved test version.
>>
>> Just an idea...
>>
>> Etienne
>>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Tim Ellison <t....@gmail.com>.
Ok, so reading everyone's post it seems that:
- we agree that there should be some way to ensure people do
not inadvertently modify String's shape, but
- there are different ways to achieve that goal, and requiring
people to acquire a lock is not favored.
I propose that we leave this thread as a further reminder to all, and if
we catch any transgressions that we revisit the mechanism.
Just to reiterate, it is certainly not my intention to prevent people
from working on String, I just want them to do so knowingly.
Regards,
Tim
Etienne Gagnon wrote:
> Geir Magnusson Jr wrote:
>> 3) Lock down function and behavior tightly with tests - that if
>> modified, tests will break, and that should raise an alarm to the
>> committer.
>
> One easy way would be to add a test that compares the String.java source
> code with a saved version, and that fails if unequal. This way, any
> change will trigger a test failure. To get past this failure, one will
> have to change both String.java and the saved test version.
>
> Just an idea...
>
> Etienne
>
--
Tim Ellison (t.p.ellison@gmail.com)
IBM Java technology centre, UK.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Etienne Gagnon <eg...@sablevm.org>.
Geir Magnusson Jr wrote:
> 3) Lock down function and behavior tightly with tests - that if
> modified, tests will break, and that should raise an alarm to the
> committer.
One easy way would be to add a test that compares the String.java source
code with a saved version, and that fails if unequal. This way, any
change will trigger a test failure. To get past this failure, one will
have to change both String.java and the saved test version.
Just an idea...
Etienne
--
Etienne M. Gagnon, Ph.D. http://www.info2.uqam.ca/~egagnon/
SableVM: http://www.sablevm.org/
SableCC: http://www.sablecc.org/
Re: [classlib] String is special
Posted by Geir Magnusson Jr <ge...@pobox.com>.
Archie Cobbs wrote:
> Etienne Gagnon wrote:
>> Tim Ellison wrote:
>>> Not really. I can add the warning, but I was looking for a way to
>>> ensure people did not mistakenly change String or did not read the
>>> doc/dev list. By failing people's commit and making them explicitly
>>> acquire the token first they have to know what they are doing.
>>
>> So, you should investigate a pre-commit script based approach. For
>> example, you could require a harmony:string-revision property on the
>> String.java file. The pre-commit script would check that any commit
>> increases this revision by one; if not, then an explicit (custom) error
>> message can be given.
>
> I kindof agree with Etienne.. it seems like you are preemptively trying
> to solve a problem that hasn't occurred yet (which is another one of
> those root-of-all-evil kind of things).
>
> Why not just put a warning in String.java, and then, if people turn out
> to be too hard-headed to heed it, bring in harder enforcement later.
> As with all open-source projects, if people aren't already being reasonable
> and communicating in the first place, then we'll have larger problems.
While I understand Tim's motivation, and I'm scared of the same thing,
I'm also not a fan of this, although I'm tired and want to give it more
consideration.
The warning won't won't solve the issue that Tim's trying to solve (as I
understand it) That someone might *accidentally* patch String, a problem
a warning won't help with. It will serve as a useful reminder for those
of us that are 'recall challenged' like me.
Again, I'll think more, but my first reaction is to
1) Remind people
2) put a warning
3) Lock down function and behavior tightly with tests - that if
modified, tests will break, and that should raise an alarm to the committer.
Being able to grok and agree to this is a requirement around here, and
will be more so in the future.
geir
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Archie Cobbs <ar...@dellroad.org>.
Etienne Gagnon wrote:
> Tim Ellison wrote:
>> Not really. I can add the warning, but I was looking for a way to
>> ensure people did not mistakenly change String or did not read the
>> doc/dev list. By failing people's commit and making them explicitly
>> acquire the token first they have to know what they are doing.
>
> So, you should investigate a pre-commit script based approach. For
> example, you could require a harmony:string-revision property on the
> String.java file. The pre-commit script would check that any commit
> increases this revision by one; if not, then an explicit (custom) error
> message can be given.
I kindof agree with Etienne.. it seems like you are preemptively trying
to solve a problem that hasn't occurred yet (which is another one of
those root-of-all-evil kind of things).
Why not just put a warning in String.java, and then, if people turn out
to be too hard-headed to heed it, bring in harder enforcement later.
As with all open-source projects, if people aren't already being reasonable
and communicating in the first place, then we'll have larger problems.
-Archie
__________________________________________________________________________
Archie Cobbs * CTO, Awarix * http://www.awarix.com
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Etienne Gagnon <eg...@sablevm.org>.
Tim Ellison wrote:
> Yes it does, your commit will fail if I have the lock; but I am abusing
> it to achieve a somewhat different goal of 'inadvertent modification'.
OK, we both agree on the "abuse" part. ;-P
> If we cannot contact the lock holder we simply break/steal the lock.
Right. But, locks should only be used for files which require
serialized modifications, such as binary blobs.
Other commit control should be made through pre-commit scripts.
> Not really. I can add the warning, but I was looking for a way to
> ensure people did not mistakenly change String or did not read the
> doc/dev list. By failing people's commit and making them explicitly
> acquire the token first they have to know what they are doing.
So, you should investigate a pre-commit script based approach. For
example, you could require a harmony:string-revision property on the
String.java file. The pre-commit script would check that any commit
increases this revision by one; if not, then an explicit (custom) error
message can be given.
Quite easy to achiev with svnlook, etc.
What do you think?
Etienne
--
Etienne M. Gagnon, Ph.D. http://www.info2.uqam.ca/~egagnon/
SableVM: http://www.sablevm.org/
SableCC: http://www.sablecc.org/
Re: [classlib] String is special
Posted by Tim Ellison <t....@gmail.com>.
Etienne Gagnon wrote:
> Hi Tim,
>
> I have an objection.
Sure, that's why I asked!
> Locking has nothing to do with "commit control".
Yes it does, your commit will fail if I have the lock; but I am abusing
it to achieve a somewhat different goal of 'inadvertent modification'.
> Allowing for svn
> locking functionality is opening a can of worms. What if somebody
> aquires a lock and loses network connectivity for a week (because of a
> Hurricane, because he forgot and went on vacation, etc.)?
If we cannot contact the lock holder we simply break/steal the lock.
I'm not proposing that the lock is used to prevent people from making
agreed upon change.
> The whole svn
> philosophy is to allow for parrallel development, instead of the
> serialized development imposed by locking based repositories (Visual
> Source Safe, RCS, etc).
I know.
> I would sugget, instead, to put a BIG WARNING at the top of String.java,
> indicating that any change must first be approved on harmony-dev@. I
> think that this would accomplish your goal in a more appropriate manner.
Not really. I can add the warning, but I was looking for a way to
ensure people did not mistakenly change String or did not read the
doc/dev list. By failing people's commit and making them explicitly
acquire the token first they have to know what they are doing.
If people think that committers will remember to do the right thing then
I'm fine with that. Unfortunately it hasn't happened to date and String
has only been in LUNI for two weeks.
Regards,
Tim
> Etienne
>
> Tim Ellison wrote:
>> To ensure that all committers can continue to update String, but that
>> they do so 'knowingly' (i.e. after considering the consequences) I'd
>> like to impose a 'positive action' pre-commit step by setting the
>> "svn:needs-lock" property on String.java.
>> ...
>> If there are no objections I'll go ahead and do this.
>
--
Tim Ellison (t.p.ellison@gmail.com)
IBM Java technology centre, UK.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: [classlib] String is special
Posted by Etienne Gagnon <eg...@sablevm.org>.
Hi Tim,
I have an objection.
Locking has nothing to do with "commit control". Allowing for svn
locking functionality is opening a can of worms. What if somebody
aquires a lock and loses network connectivity for a week (because of a
Hurricane, because he forgot and went on vacation, etc.)? The whole svn
philosophy is to allow for parrallel development, instead of the
serialized development imposed by locking based repositories (Visual
Source Safe, RCS, etc).
I would sugget, instead, to put a BIG WARNING at the top of String.java,
indicating that any change must first be approved on harmony-dev@. I
think that this would accomplish your goal in a more appropriate manner.
Etienne
Tim Ellison wrote:
> To ensure that all committers can continue to update String, but that
> they do so 'knowingly' (i.e. after considering the consequences) I'd
> like to impose a 'positive action' pre-commit step by setting the
> "svn:needs-lock" property on String.java.
>...
> If there are no objections I'll go ahead and do this.
--
Etienne M. Gagnon, Ph.D. http://www.info2.uqam.ca/~egagnon/
SableVM: http://www.sablevm.org/
SableCC: http://www.sablecc.org/
Re: Java 5 String APIs (code points, StringBuilder, etc) RE: [classlib]
String is special
Posted by Geir Magnusson Jr <ge...@pobox.com>.
Or just put the test in java.lang as an implementation test.
We should resolve this. Among others, we're going to have spec tests,
which Tim and others rightly point out that we need to be careful with
and keep out of the boot classpath to ensure that tests happen in the
context of userland.
However, with a good understanding of the limits, we should be also able
to have the 'easy to use' same-package tests for implementation.
In that vein, we could also create a helper framework that lets us write
tests in java.lang (for example) and it transforms into the helper Tim
suggests automatically...
geir
Tim Ellison wrote:
> I suggest you write a helper method to create a String with that
> constructor using reflection (setAccessible(true)).
>
> Regards,
> Tim
>
> Nathan Beyer wrote:
>> Since I probably share some responsibility in the "String is special" topic
>> being brought up, I wanted to try out the "golden ticket" bit for some
>> further enhancements.
>>
>> I've made some changes to String to add the new Java 5 APIs related to code
>> points, StringBuilder, etc.
>>
>> I've posted a proposed patch on this issue [1]. I'd already opened this bug.
>> The patch contains updates to String and StringTest.
>>
>> There is one concern that I have about the code and the test (and all String
>> tests we have in general). There's the possibility of a String being
>> constructed with a non-zero offset; there's a package-private constructor
>> String(int start, int len, char[] data). I haven't found any other way to
>> setup this variant and I'd like to test all of the methods when the offset
>> is greater than 0, but this requires the test case to be in the "java.lang"
>> package. I know this has been an issue that been discussed on and off, but
>> didn't think any concrete solution had been arrived at for doing this and I
>> haven't seen any test cases in the java package spaces.
>>
>> Please let me know if there are any questions or if any more information is
>> needed. Thanks.
>>
>> -Nathan
>>
>>
>> ---------------------------------------------------------------------
>> Terms of use : http://incubator.apache.org/harmony/mailing.html
>> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
>> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>>
>>
>
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Re: Java 5 String APIs (code points, StringBuilder, etc) RE: [classlib]
String is special
Posted by Tim Ellison <t....@gmail.com>.
I suggest you write a helper method to create a String with that
constructor using reflection (setAccessible(true)).
Regards,
Tim
Nathan Beyer wrote:
> Since I probably share some responsibility in the "String is special" topic
> being brought up, I wanted to try out the "golden ticket" bit for some
> further enhancements.
>
> I've made some changes to String to add the new Java 5 APIs related to code
> points, StringBuilder, etc.
>
> I've posted a proposed patch on this issue [1]. I'd already opened this bug.
> The patch contains updates to String and StringTest.
>
> There is one concern that I have about the code and the test (and all String
> tests we have in general). There's the possibility of a String being
> constructed with a non-zero offset; there's a package-private constructor
> String(int start, int len, char[] data). I haven't found any other way to
> setup this variant and I'd like to test all of the methods when the offset
> is greater than 0, but this requires the test case to be in the "java.lang"
> package. I know this has been an issue that been discussed on and off, but
> didn't think any concrete solution had been arrived at for doing this and I
> haven't seen any test cases in the java package spaces.
>
> Please let me know if there are any questions or if any more information is
> needed. Thanks.
>
> -Nathan
>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>
--
Tim Ellison (t.p.ellison@gmail.com)
IBM Java technology centre, UK.
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
RE: Java 5 String APIs (code points, StringBuilder, etc) RE: [classlib] String is special
Posted by Nathan Beyer <nb...@kc.rr.com>.
BTW: Here's a direct link to the patch -
http://issues.apache.org/jira/secure/attachment/12325649/String_java_5_metho
ds3.txt
> -----Original Message-----
> From: Nathan Beyer [mailto:nbeyer@kc.rr.com]
> Sent: Thursday, April 20, 2006 9:30 PM
> To: harmony-dev@incubator.apache.org
> Subject: Java 5 String APIs (code points, StringBuilder, etc) RE:
> [classlib] String is special
>
> Since I probably share some responsibility in the "String is special"
> topic
> being brought up, I wanted to try out the "golden ticket" bit for some
> further enhancements.
>
> I've made some changes to String to add the new Java 5 APIs related to
> code
> points, StringBuilder, etc.
>
> I've posted a proposed patch on this issue [1]. I'd already opened this
> bug.
> The patch contains updates to String and StringTest.
>
> There is one concern that I have about the code and the test (and all
> String
> tests we have in general). There's the possibility of a String being
> constructed with a non-zero offset; there's a package-private constructor
> String(int start, int len, char[] data). I haven't found any other way to
> setup this variant and I'd like to test all of the methods when the offset
> is greater than 0, but this requires the test case to be in the
> "java.lang"
> package. I know this has been an issue that been discussed on and off, but
> didn't think any concrete solution had been arrived at for doing this and
> I
> haven't seen any test cases in the java package spaces.
>
> Please let me know if there are any questions or if any more information
> is
> needed. Thanks.
>
> -Nathan
>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org
Java 5 String APIs (code points, StringBuilder, etc) RE: [classlib] String is special
Posted by Nathan Beyer <nb...@kc.rr.com>.
Since I probably share some responsibility in the "String is special" topic
being brought up, I wanted to try out the "golden ticket" bit for some
further enhancements.
I've made some changes to String to add the new Java 5 APIs related to code
points, StringBuilder, etc.
I've posted a proposed patch on this issue [1]. I'd already opened this bug.
The patch contains updates to String and StringTest.
There is one concern that I have about the code and the test (and all String
tests we have in general). There's the possibility of a String being
constructed with a non-zero offset; there's a package-private constructor
String(int start, int len, char[] data). I haven't found any other way to
setup this variant and I'd like to test all of the methods when the offset
is greater than 0, but this requires the test case to be in the "java.lang"
package. I know this has been an issue that been discussed on and off, but
didn't think any concrete solution had been arrived at for doing this and I
haven't seen any test cases in the java package spaces.
Please let me know if there are any questions or if any more information is
needed. Thanks.
-Nathan
---------------------------------------------------------------------
Terms of use : http://incubator.apache.org/harmony/mailing.html
To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
For additional commands, e-mail: harmony-dev-help@incubator.apache.org