I should point out that this form I demonstrate below is only used when
there are continuation lines on the prefix to the code block
(conditionals, method declarations, loop controls, etc.), so:
if (some simple condition) {
...
}
and:
if (some complex condition &&
some additional condition)
{
...
}
but, not:
if (some single line condition)
{
// blech
...
}
The reason for this is that the standard indentation would recommend:
if (some complex condition &&
some additional condition) {
// code block
}
or
void foomethod(int sx, int sy,
int dx, int dy) {
// code block
}
which may be more compact, but the lack of a breaking line means you
have to vary the indentation of the declarations/conditionals and as a
result they don't line up nicely which makes them harder to scan if they
are all related (and frequently in graphics code you end up with a
series of very similar arguments or conditionals that are more readable
if they line up nicely), and the only indication of when the multiple
lines of continued declaration/conditionals end and the body of the
method begins is the number of spaces - noting that frequently the
indentation on lines in practice is just wrong so this form makes it
hard to distinguish between "that line is a continuation line" and
"someone indented that line wrong"...
...jim
On 3/23/16 5:14 PM, Jim Graham wrote:
For the record, in many places in the 2D code we also adopt a slight
extension of the indentation rules such that the below might be written as:
public ByteBandedRaster(SampleModel sampleModel,
DataBufferByte dataBuffer,
Point origin)
{
// body lines of the method...
}
with the open curly brace on the following line so that it more visually
points out the beginning of the body block of the method and it's easy
to find the start/end of the block by sighting down the left margin. The
official policy is for the "{" to be at the end of the previous line
after "Point origin) {" and as more new engineers work on the code and
follow the official rules, the above form becomes less common. (Sad
face since I particularly like the above form...)
...jim
On 3/22/16 4:10 PM, Phil Race wrote:
Ajit,
There is also some odd indentation in ByteBandedRaster.java which is not
yours but
98 public ByteBandedRaster(SampleModel sampleModel,
99 DataBufferByte dataBuffer,
100 Point origin) {
This appears to be the result of someone using tabs a long time ago.
Since you are touching the method signature lines anyway may be
better fixed so we have these aligned
public ByteBandedRaster(SampleModel sampleModel,
DataBufferByte dataBuffer,
Point origin) {
[not sure if that will make it through mail as intended].
Here in Raster.java, the first condition now seems to be redundant ..
890 if (dataType == DataBuffer.TYPE_BYTE &&
891 dataBuffer instanceof DataBufferByte &&
-phil.
On 03/22/2016 03:28 PM, Jim Graham wrote:
Hi Ajit,
Most of your if statements are spaced wrong. There should be a space
between "if" and the parentheses. I'll review more later, but I
noticed that issue in the first couple of files I looked at...
...jim
On 3/15/16 7:05 AM, Ajit Ghaisas wrote:
Hi,
Thanks Sergey and Jim for suggestions.
I have made the data specific Raster constructors type safe
now. Also, I have modified all Raster creations in Raster.java to
support custom DataBuffer classes.
Please review the changes present in updated webrev :
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.02/
Regards,
Ajit
-----Original Message-----
From: Jim Graham
Sent: Friday, March 11, 2016 2:40 AM
To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518:
Creation of a WritableRaster with a custom DataBuffer causes
erroneous Exception
Yes, those constructors should be type-safe. Perhaps that was done
to save code in the caller, but that is a small price to pay to avoid
errors in the future, and it makes sure there is a backup plan for
different kinds of buffers...
...jim
On 3/10/16 4:48 AM, Sergey Bylokhov wrote:
Hi, Ajit.
What about other cases in Raster.java, where the DataBuffer is passed
as a parameter? Probably we can simplify the code and find all such
issues if we changes the ByteInterleavedRaster/etc. For example:
ByteInterleavedRaster(SampleModel sampleModel,DataBuffer
dataBuffer,...) to
ByteInterleavedRaster(SampleModel sampleModel,DataBufferByte
dataBuffer,...)
Because we throw an exception in such cases anyway:
if (!(dataBuffer instanceof DataBufferByte)) {
throw new RasterFormatException("ByteInterleavedRasters must
have "
+ "byte DataBuffers");
}
And the compiler will help us, what everybody think about it?
On 09.03.16 17:38, Ajit Ghaisas wrote:
Hi,
Modified the test and added check for
MultiPixelPackedSampleModel
condition.
Please review updated webrev :
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.01/
Regards,
Ajit
-----Original Message-----
From: Sergey Bylokhov
Sent: Wednesday, March 09, 2016 4:06 PM
To: Ajit Ghaisas; awt-...@openjdk.java.net; Semyon Sadetsky; Ambarish
Rapte; 2d-dev
Subject: Re: [9] Review-request for 6353518: Creation of a
WritableRaster with a custom DataBuffer causes erroneous Exception
Changes for 2d area.(cc)
On 07.03.16 11:20, Ajit Ghaisas wrote:
Hi,
Bug : https://bugs.openjdk.java.net/browse/JDK-6353518
Issue : (Text from bug description)
An attempt to create a WritableRaster via
Raster.createWritableRaster(SampleModel sm, DataBuffer db, Point
location) using a custom DataBuffer causes an erroneous
RasterFormatException.
Apparently the reason for this bug is that IntegerComponentRaster
insists on being passed an instance of DataBufferInt rather than
just a DataBuffer with a DataType of TYPE_INT.
This is quite annoying since DataBufferInt is declared final and
thus cannot be extended.
Fix :
The last line of Raster.createWritableRaster() method already has a
way to handle generic case if the input does not match any of the
cases in switch statements in that method.
The fact that " *InterleavedRaster() constructors throw exception
if DataBuffer passed to them does not match the expected type" was
ignored earlier.
This fix adds a check of "DataBuffer type" before creating
respective
*InterleavedRaster() objects.
It constructs the SunWritableRaster in case DataBuffer type does not
match any data specific DataBuffer classes (DataBufferByte,
DataBufferUShort, DataBufferInt)
Request to review webrev containing this fix :
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.00/
Regards,
Ajit
--
Best regards, Sergey.
