[
http://issues.apache.org/jira/browse/DERBY-336?page=comments#action_12314532 ]
Dyre Tjeldvoll commented on DERBY-336:
--------------------------------------
Thanks for your comments, David!
I see what you are saying about FILE_CANNOT_REMOVE_FILE, but look at how it is
used other places in the code:
./java/engine/org/apache/derby/impl/store/raw/data/RFResource.java:261:
SQLState.FILE_CANNOT_REMOVE_FILE, fileToGo);
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1077:
newException(SQLState.FILE_CANNOT_REMOVE_FILE, se, file,
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1102:
SQLState.FILE_CANNOT_REMOVE_FILE, ioe2, file, ioe);
./java/engine/org/apache/derby/impl/store/raw/data/RAFContainer.java:1107:
SQLState.FILE_CANNOT_REMOVE_FILE, se, file, stub);
./java/engine/org/apache/derby/iapi/reference/SQLState.java:489: String
FILE_CANNOT_REMOVE_FILE = "XSDF4.S";
As you can see, this error message is used both with one and TWO nested
exceptions (in much the same way as FILE_CREATE_NO_CLEANUP),
so you cannot remove the additional parameter from the message string. I agree
that you can choose to supply the same exception both as the nested exception,
and as a parameter, but I doubt that this is why the error message is written
the way it is... Also, notice the first use of this message, where NO nested
exception is supplied!
---
I think you are correct about using a string argument. However, I was cautioned
about converting arguments to string before it is necessary, as this could lead
to performance problems, (if the exception is caught, the conversion to string
is unnecessary). But in this particular case I doubt that performance is
relevant, so I'll consider changing it...
---
I understand your concerns about the dummy overloads, - it is a non-standard
and non-intuitive construct. I was not sure if I should include them in the
patch or just use them locally to find the StandardException usages that needed
to be fixed. In the end, I decided to include them since it is easy to make
another such mistake later, (and you will not be warned about it).
> The wrong overload of StandardException::newException() is used in some cases
> -----------------------------------------------------------------------------
>
> Key: DERBY-336
> URL: http://issues.apache.org/jira/browse/DERBY-336
> Project: Derby
> Type: Bug
> Environment: Any
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Priority: Trivial
> Attachments: derby-336.diff, derby-336.stat, derbyall_report.txt
>
> When looking at DERBY-128 it became clear that the wrong overload of
> StandardException::newException() was used when reporting
> SQLState.SERVICE_DIRECTORY_CREATE_ERROR. The message string only takes one
> parameter so only one additional parameter (other than Throwable) should be
> used:
> PersistentServiceImpl.java:676
> throw
> StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR,
>
> serviceDirectory, null);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Object)? Or
> StandardException.newException(String, Throwable, Object)? With the
> IOException as
> // Throwable?
> PersistentServiceImpl.java:692
> throw
> StandardException.newException(SQLState.SERVICE_DIRECTORY_CREATE_ERROR, name,
> t);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Throwable, Object)?
> BaseDataFileFactory.java:279
> throw StandardException.newException(
> SQLState.SERVICE_DIRECTORY_CREATE_ERROR, dataDirectory, ioe);
> // Calls StandardException.newException(String, Object, Object)
> // Should call StandardException.newException(String, Throwable, Object)?
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
http://www.atlassian.com/software/jira
