You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cayenne.apache.org by Andrus Adamchik <an...@objectstyle.org> on 2007/10/15 00:14:09 UTC
Re: svn commit: r584611 - in /cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne: ObjectId.java access/jdbc/SQLTemplateAction.java util/IndexPropertyList.java
Hi Kevin,
Nice work cleaning up the code tonight!
One question - why do you think the break statement below is not
needed? (Keep in mind that it is past 1am in my TZ right now, so I
may be asking stupid/obvious stuff :-))
Andrus
On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:
> Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
> src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
> URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
> cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
> jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
> ======================================================================
> ========
> --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
> (original)
> +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
> Oct 14 14:30:39 2007
> @@ -159,10 +157,7 @@
> t1);
> }
> finally {
> - if (iteratedResult) {
> - break;
> - }
> - else {
> + if (!iteratedResult) {
> resultSet.close();
> }
> }
Re: svn commit: r584611 - in
/cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/main/java/org/
apache/cayenne: ObjectId.java access/jdbc/SQLTemplateAction.java
util/IndexPropertyList.java
Posted by Kevin Menard <km...@servprise.com>.
Then I'd probably say that needs to be cleaned up. I was alerted to the
potential issue from IntelliJ's code inspector. Then looking at the code,
it was structured a bit oddly.
I could understand breaking on an exception, but from a cleanup block that
runs at a non-deterministic time, it seems very suspect to me. We can
revert the commit, but it still looks like a place that deserves some amount
of attention.
--
Kevin
On 10/15/07 3:23 AM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
> "break" is not really a noop, as we are dealing with "while(true)"
> loop. This may not be coded well, but if I remember correctly the
> intent was to get out of the loop for iterated results, without
> checking for possible update counts. Sure we should not probably put
> it in "finally".
>
> Andrus
>
>
> On Oct 15, 2007, at 1:27 AM, Kevin Menard wrote:
>
>> Because it was in a finally block. It appeared as though the break
>> was
>> added because returning from a finally is a big no-no. But, the
>> break was
>> essentially a noop for that one case. By inverting the logic and
>> dealing
>> with only the condition you care about, the other is a de facto noop.
>>
>> Hopefully I explained that well enough. It's late for me, too.
>> I'm in my
>> German hotel room whittling away time.
>>
>> --
>> Kevin
>>
>>
>> On 10/14/07 6:14 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>>
>>> Hi Kevin,
>>>
>>> Nice work cleaning up the code tonight!
>>>
>>> One question - why do you think the break statement below is not
>>> needed? (Keep in mind that it is past 1am in my TZ right now, so I
>>> may be asking stupid/obvious stuff :-))
>>>
>>> Andrus
>>>
>>>
>>> On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:
>>>
>>>> Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
>>>> src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>>>> URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
>>>> cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
>>>> jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
>>>> ====================================================================
>>>> ==
>>>> ========
>>>> --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>>>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>>>> (original)
>>>> +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>>>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
>>>> Oct 14 14:30:39 2007
>>>> @@ -159,10 +157,7 @@
>>>> t1);
>>>> }
>>>> finally {
>>>> - if (iteratedResult) {
>>>> - break;
>>>> - }
>>>> - else {
>>>> + if (!iteratedResult) {
>>>> resultSet.close();
>>>> }
>>>> }
>>>
>>
>> --
>>
>>
>>
>
--
Re: svn commit: r584611 - in /cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/main/java/org/ apache/cayenne: ObjectId.java access/jdbc/SQLTemplateAction.java util/IndexPropertyList.java
Posted by Andrus Adamchik <an...@objectstyle.org>.
"break" is not really a noop, as we are dealing with "while(true)"
loop. This may not be coded well, but if I remember correctly the
intent was to get out of the loop for iterated results, without
checking for possible update counts. Sure we should not probably put
it in "finally".
Andrus
On Oct 15, 2007, at 1:27 AM, Kevin Menard wrote:
> Because it was in a finally block. It appeared as though the break
> was
> added because returning from a finally is a big no-no. But, the
> break was
> essentially a noop for that one case. By inverting the logic and
> dealing
> with only the condition you care about, the other is a de facto noop.
>
> Hopefully I explained that well enough. It's late for me, too.
> I'm in my
> German hotel room whittling away time.
>
> --
> Kevin
>
>
> On 10/14/07 6:14 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
>
>> Hi Kevin,
>>
>> Nice work cleaning up the code tonight!
>>
>> One question - why do you think the break statement below is not
>> needed? (Keep in mind that it is past 1am in my TZ right now, so I
>> may be asking stupid/obvious stuff :-))
>>
>> Andrus
>>
>>
>> On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:
>>
>>> Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
>>> src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>>> URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
>>> cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
>>> jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
>>> ====================================================================
>>> ==
>>> ========
>>> --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>>> (original)
>>> +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
>>> Oct 14 14:30:39 2007
>>> @@ -159,10 +157,7 @@
>>> t1);
>>> }
>>> finally {
>>> - if (iteratedResult) {
>>> - break;
>>> - }
>>> - else {
>>> + if (!iteratedResult) {
>>> resultSet.close();
>>> }
>>> }
>>
>
> --
>
>
>
Re: svn commit: r584611 - in
/cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/main/java/org/
apache/cayenne: ObjectId.java access/jdbc/SQLTemplateAction.java
util/IndexPropertyList.java
Posted by Kevin Menard <km...@servprise.com>.
Because it was in a finally block. It appeared as though the break was
added because returning from a finally is a big no-no. But, the break was
essentially a noop for that one case. By inverting the logic and dealing
with only the condition you care about, the other is a de facto noop.
Hopefully I explained that well enough. It's late for me, too. I'm in my
German hotel room whittling away time.
--
Kevin
On 10/14/07 6:14 PM, "Andrus Adamchik" <an...@objectstyle.org> wrote:
> Hi Kevin,
>
> Nice work cleaning up the code tonight!
>
> One question - why do you think the break statement below is not
> needed? (Keep in mind that it is past 1am in my TZ right now, so I
> may be asking stupid/obvious stuff :-))
>
> Andrus
>
>
> On Oct 15, 2007, at 12:30 AM, kmenard@apache.org wrote:
>
>> Modified: cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/
>> src/main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>> URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/
>> cayenne-jdk1.4-unpublished/src/main/java/org/apache/cayenne/access/
>> jdbc/SQLTemplateAction.java?rev=584611&r1=584610&r2=584611&view=diff
>> ======================================================================
>> ========
>> --- cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java
>> (original)
>> +++ cayenne/main/trunk/framework/cayenne-jdk1.4-unpublished/src/
>> main/java/org/apache/cayenne/access/jdbc/SQLTemplateAction.java Sun
>> Oct 14 14:30:39 2007
>> @@ -159,10 +157,7 @@
>> t1);
>> }
>> finally {
>> - if (iteratedResult) {
>> - break;
>> - }
>> - else {
>> + if (!iteratedResult) {
>> resultSet.close();
>> }
>> }
>
--