Hello.
I decide to postpone modification of dblook.
Modification itself is not difficult.
However , it is needed to know all about SYSTEM table , to check modification was correctly done.
It is a little hard for me to understand all about SYSTEM table right now.
//This may be suitable for my next task ...
Best regards.
/*
Tomohito Nakayama
[EMAIL PROTECTED]
[EMAIL PROTECTED] Naka
http://www5.ocn.ne.jp/~tomohito/TopPage.html*/
----- Original Message ----- From: "TomohitoNakayama" <[EMAIL PROTECTED]>
To: "Derby Development" <derby-dev@db.apache.org>
Sent: Wednesday, May 18, 2005 11:36 PM
Subject: Re: Patch again again for DERBY-167.
Hello.
Daniel John Debrunner wrote:
1st:
The Derby "dblook" utility will have to be modified to account for the "BY DEFAULT" keyword--right now, it generates all autoincrement columns using the "ALWAYS" keyword. See org/apache/derby/impl/tools/dblook/DB_Table.java, in the "reinsateAutoIncrement" method. Luckily, this change should be fairly easy given the changes in your patch.
I see. I will modify dblook. Thank you for your notifying :D
The dblook changes could be a separate patch. Incremental development is a good process, rather than requiring all the related changes to be in one huge patch.
Well .... I just have started modification of dblook. Then I will include it. But if encountered some problem , I will postpone it for other task.
if (defaultInfo != null && ! colDesc.isAutoincrement())
it would be good to add some descriptive comment. The issue here is that the test is two negative conditions 'anded (&&)' together. Somewhat hard for humans (well at least me) to understand quickly. A comment is also useful for the original coder to help them ensure they have the correct condition.
Well .... This condition is surely difficult. Comment will be needed . I will add.
Sometimes comment can be barrier for reading code. Therefore, I hesitate to write comment.
BITS_MASK_IS_DEFAULTVALUE_AUTOINC should be 'final' if it is intended to be a constant.
In DefaultInfoImpl the calculating of definitionBits and isDefaultValueAutoinc using those static methods seems too complex. If there are ever a few more types to calculate we end up with a lot of code to deal with it. Why not just have a single instance field representing the type (matching your defintion bits), remove isDefaultValueAutoinc field. Then the read/writeExternal methods just write the type directly, and the isDefaultValueAutoinc method is something like
+ public boolean isDefaultValueAutoinc(){ + return (type & BITS_MASK_IS_DEFAULTVALUE_AUTOINC) != 0; + }
I hesitated calculating each time calling isDefaultValueAutoinc() method. Well .... But its very small price. Price of calculating each time will correspond to simple code. I will modify it.
Best regards.
/*
Tomohito Nakayama [EMAIL PROTECTED] [EMAIL PROTECTED]
Naka http://www5.ocn.ne.jp/~tomohito/TopPage.html
*/
----- Original Message ----- From: "Daniel John Debrunner" <[EMAIL PROTECTED]>
To: "Derby Development" <derby-dev@db.apache.org>
Sent: Wednesday, May 18, 2005 10:22 PM
Subject: Re: Patch again again for DERBY-167.
TomohitoNakayama wrote:
Hello.
1st:
The Derby "dblook" utility will have to be modified to account for the "BY DEFAULT" keyword--right now, it generates all autoincrement columns using the "ALWAYS" keyword. See org/apache/derby/impl/tools/dblook/DB_Table.java, in the "reinsateAutoIncrement" method. Luckily, this change should be fairly easy given the changes in your patch.
I see. I will modify dblook. Thank you for your notifying :D
The dblook changes could be a separate patch. Incremental development is a good process, rather than requiring all the related changes to be in one huge patch.
2nd:
Second (minor):
I noticed that in DefaultInfoImpl.java, the two new methods (getDefinitionBitsValue and calcIsDefaultValueAutoinc) are declared as static--is there a reason for that? I made them non-static and everything compiled, so I'm just wondering if this was intentional?
Yes. That was intentional.
I prefer static method because it have narrower scope than instance method.
I agree, static methods are the correct type here.
I would encourage you (and anyone else) to keep any description (or modified version of it) from the original patch e-mail in newer e-mails with modified patches. This makes it easier for the committers to understand and track patches. E.g. you last Derby-167 patch just had the patch attached, you should keep any description from the first e-mail with the patch. Another reason is that the description may have changed due to comments etc.
In general the patch looks good, thanks for making changes for my previous feedback.
Some minor comments on the patch:
In ResultSetNode and ColumnDefintionNode for this line you changed
if (defaultInfo != null && ! colDesc.isAutoincrement())
it would be good to add some descriptive comment. The issue here is that the test is two negative conditions 'anded (&&)' together. Somewhat hard for humans (well at least me) to understand quickly. A comment is also useful for the original coder to help them ensure they have the correct condition.
BITS_MASK_IS_DEFAULTVALUE_AUTOINC should be 'final' if it is intended to be a constant.
In DefaultInfoImpl the calculating of definitionBits and isDefaultValueAutoinc using those static methods seems too complex. If there are ever a few more types to calculate we end up with a lot of code to deal with it. Why not just have a single instance field representing the type (matching your defintion bits), remove isDefaultValueAutoinc field. Then the read/writeExternal methods just write the type directly, and the isDefaultValueAutoinc method is something like
+ public boolean isDefaultValueAutoinc(){ + return (type & BITS_MASK_IS_DEFAULTVALUE_AUTOINC) != 0; + }
Dan.
-- No virus found in this incoming message. Checked by AVG Anti-Virus. Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17
-- No virus found in this outgoing message. Checked by AVG Anti-Virus. Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17
-- No virus found in this incoming message. Checked by AVG Anti-Virus. Version: 7.0.308 / Virus Database: 266.11.12 - Release Date: 2005/05/17
-- No virus found in this outgoing message. Checked by AVG Anti-Virus. Version: 7.0.322 / Virus Database: 266.11.12 - Release Date: 2005/05/17
