Fix potential bug in FastSharedBufferReader::getConsecutiveData

Fix potential bug in FastSharedBufferReader::getConsecutiveData

The loop has a logical error: it can read the SharedBuffer further after the
desired number of bytes already read. This would cause assertion - and other
sort of funny things in release - if callers want to read from the end of the
next to last segment overlapping to the last one.

Besides the fix the unit test went through some refactor.

BUG=462733
TEST=readOverlappingLastSegmentBoundary

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193404

[kbalazs, 2015-03-17 18:28:25]

b.kelemen@samsung.com changed reviewers:
+ hclam@chromium.org
- hclam@chroimium.org

[kbalazs, 2015-03-19 23:43:13]

b.kelemen@samsung.com changed reviewers:
+ pkasting@chromium.org

[kbalazs, 2015-03-19 23:43:14]

PTAL

[Alpha Left Google, 2015-03-19 23:52:14]

Good catch. LGTM.

[Peter Kasting, 2015-03-20 01:10:43]

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:52: // Return a
pointer into |m_data| if the request doesn't span segments.
We can simplify the function by replacing everything from here on with:

    char* out_ptr = m_segment;
    for (char* copy_dest = buffer; ; copy_dest += m_segmentLength) {
        m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
        ASSERT(m_segmentLength);
        if (out_ptr == buffer)
            memcpy(copy_dest, m_segment, m_segmentLength);
        if (length <= m_segmentLength)
            return out_ptr;
        if (out_ptr != buffer) {
            out_ptr = buffer;
            memcpy(copy_dest, m_segment, m_segmentLength);
        }
        m_dataPosition += m_segmentLength;
        length -= m_segmentLength;
    }

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:76:
ASSERT_NOT_REACHED();
I think these two lines will trigger MSVC warning C4702, and you should remove
them.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReaderTest.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:41: const unsigned
segmentSize = 0x1000;
I think you should make the existing constant in SharedBuffer.cpp public and
then use it here.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:106:
TEST(FastSharedBufferReaderTest, readOverlappingLastSegmentBoundary)
This test should have comments describing the case it's testing, specifically,
that we need to do a read from (before the last segment) to (the very end of the
last segment).

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:111: // Append it
one segment at a time.
Why is this necessary?  If seems like SharedBuffer::append() should do the same
thing whether called once with a larger block or twice with a smaller.

[kbalazs, 2015-03-20 20:20:43]

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:52: // Return a
pointer into |m_data| if the request doesn't span segments.
On 2015/03/20 01:10:42, Peter Kasting wrote:
> We can simplify the function by replacing everything from here on with:
> 
>     char* out_ptr = m_segment;
>     for (char* copy_dest = buffer; ; copy_dest += m_segmentLength) {
>         m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
>         ASSERT(m_segmentLength);
>         if (out_ptr == buffer)
>             memcpy(copy_dest, m_segment, m_segmentLength);
>         if (length <= m_segmentLength)
>             return out_ptr;
>         if (out_ptr != buffer) {
>             out_ptr = buffer;
>             memcpy(copy_dest, m_segment, m_segmentLength);
>         }
>         m_dataPosition += m_segmentLength;
>         length -= m_segmentLength;
>     }

Quite frankly I don't see how is this simpler. To me separating the cases when
pointer to m_segment is returned and when the data is copied to output buffer is
much easier to reason about.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:76:
ASSERT_NOT_REACHED();
On 2015/03/20 01:10:42, Peter Kasting wrote:
> I think these two lines will trigger MSVC warning C4702, and you should remove
> them.

Acknowledged.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReaderTest.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:41: const unsigned
segmentSize = 0x1000;
On 2015/03/20 01:10:42, Peter Kasting wrote:
> I think you should make the existing constant in SharedBuffer.cpp public and
> then use it here.

Acknowledged.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:106:
TEST(FastSharedBufferReaderTest, readOverlappingLastSegmentBoundary)
On 2015/03/20 01:10:42, Peter Kasting wrote:
> This test should have comments describing the case it's testing, specifically,
> that we need to do a read from (before the last segment) to (the very end of
the
> last segment).

Actually the read doesn't have to go for the very end, just overlap the one
before last and the last segments. I will add comment.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:111: // Append it
one segment at a time.
On 2015/03/20 01:10:42, Peter Kasting wrote:
> Why is this necessary?  If seems like SharedBuffer::append() should do the
same
> thing whether called once with a larger block or twice with a smaller.

Correct.

[Peter Kasting, 2015-03-20 20:29:34]

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:52: // Return a
pointer into |m_data| if the request doesn't span segments.
On 2015/03/20 20:20:42, kbalazs wrote:
> On 2015/03/20 01:10:42, Peter Kasting wrote:
> > We can simplify the function by replacing everything from here on with:
> > 
> >     char* out_ptr = m_segment;
> >     for (char* copy_dest = buffer; ; copy_dest += m_segmentLength) {
> >         m_segmentLength = m_data->getSomeData(m_segment, m_dataPosition);
> >         ASSERT(m_segmentLength);
> >         if (out_ptr == buffer)
> >             memcpy(copy_dest, m_segment, m_segmentLength);
> >         if (length <= m_segmentLength)
> >             return out_ptr;
> >         if (out_ptr != buffer) {
> >             out_ptr = buffer;
> >             memcpy(copy_dest, m_segment, m_segmentLength);
> >         }
> >         m_dataPosition += m_segmentLength;
> >         length -= m_segmentLength;
> >     }
> 
> Quite frankly I don't see how is this simpler. To me separating the cases when
> pointer to m_segment is returned and when the data is copied to output buffer
is
> much easier to reason about.

Fair enough.  "Simpler" in this case means that it's a half-dozen lines shorter
and we're not duplicating the getSomeData() call, but it does make the control
flow a bit thornier.  I'm fine leaving it.

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:59: char* dest =
buffer;
Nit: Convert the while to a for so you can move this declaration inside it
("declare variables in the narrowest possible scope").

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReaderTest.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:106:
TEST(FastSharedBufferReaderTest, readOverlappingLastSegmentBoundary)
On 2015/03/20 20:20:42, kbalazs wrote:
> On 2015/03/20 01:10:42, Peter Kasting wrote:
> > This test should have comments describing the case it's testing,
specifically,
> > that we need to do a read from (before the last segment) to (the very end of
> the
> > last segment).
> 
> Actually the read doesn't have to go for the very end, just overlap the one
> before last and the last segments.

Are you sure?  I read the original code and AFAICT it wouldn't have a problem
unless the read in question went to the end of the last buffer:

* We'd first getSomeData() from the second-to-last segment.
* Then the loop would start.  We'd copy that data to the buffer, and request
data from the start of the last segment.  This would return the entire last
segment.
* The loop would iterate.  We'd copy min(length, m_segmentLength) bytes.  More
critically, we'd also increment m_dataPosition by that minimum value.  So if
length < m_segmentLength at this point, m_dataPosition would now be somewhere in
the middle of the last segment.
* We'd getSomeData() again.  Since m_dataPosition wouldn't be off-the-end, the
call would succeed.

This is why the existing test didn't show the issue, despite the fact that it
does read across the boundary of the last two segments.

[kbalazs, 2015-03-20 20:37:44]

> 
> Are you sure?  I read the original code and AFAICT it wouldn't have a problem
> unless the read in question went to the end of the last buffer:
> 
> * We'd first getSomeData() from the second-to-last segment.
> * Then the loop would start.  We'd copy that data to the buffer, and request
> data from the start of the last segment.  This would return the entire last
> segment.
> * The loop would iterate.  We'd copy min(length, m_segmentLength) bytes.  More
> critically, we'd also increment m_dataPosition by that minimum value.  So if
> length < m_segmentLength at this point, m_dataPosition would now be somewhere
in
> the middle of the last segment.
> * We'd getSomeData() again.  Since m_dataPosition wouldn't be off-the-end, the
> call would succeed.
> 
> This is why the existing test didn't show the issue, despite the fact that it
> does read across the boundary of the last two segments.

Yes, you are right. It would just cycle a bit more than necessary.

[Alpha Left Google, 2015-03-20 20:41:27]

On 2015/03/20 20:37:44, kbalazs wrote:
> > 
> > Are you sure?  I read the original code and AFAICT it wouldn't have a
problem
> > unless the read in question went to the end of the last buffer:
> > 
> > * We'd first getSomeData() from the second-to-last segment.
> > * Then the loop would start.  We'd copy that data to the buffer, and request
> > data from the start of the last segment.  This would return the entire last
> > segment.
> > * The loop would iterate.  We'd copy min(length, m_segmentLength) bytes. 
More
> > critically, we'd also increment m_dataPosition by that minimum value.  So if
> > length < m_segmentLength at this point, m_dataPosition would now be
somewhere
> in
> > the middle of the last segment.
> > * We'd getSomeData() again.  Since m_dataPosition wouldn't be off-the-end,
the
> > call would succeed.
> > 
> > This is why the existing test didn't show the issue, despite the fact that
it
> > does read across the boundary of the last two segments.
> 
> Yes, you are right. It would just cycle a bit more than necessary.

The test that was there could have revealed the issue if it made the last read
for the remaining bytes. Otherwise the test already covered the case when a read
spans across the last and the second to last segments.

[kbalazs, 2015-03-25 19:38:10]

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/1/Source/platform/image-decod...
Source/platform/image-decoders/FastSharedBufferReader.cpp:63: m_dataPosition +=
copy;
I also introduced an error. m_dataPosition should not be changed if we are not
continue reading. I will make some refactor too make this code easier to read.

[kbalazs, 2015-03-25 19:41:00]

b.kelemen@samsung.com changed reviewers:
+ adamk@chromium.org

[kbalazs, 2015-03-25 19:41:00]

pkasting@chromium.org: PTAL
adamk@chromium.org: Please rubber-stamp the platform/ parts

[kbalazs, 2015-03-25 19:41:49]

> mailto:adamk@chromium.org: Please rubber-stamp the platform/ parts

I mean the SharedBuffer parts.

[Peter Kasting, 2015-03-25 19:53:11]

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/SharedB...
File Source/platform/SharedBuffer.cpp (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/SharedB...
Source/platform/SharedBuffer.cpp:51: return position & segmentPositionMask;
This will be safer if you do:

  return position % SharedBuffer::kSegmentSize;

That way if the segment size is changed to a non-power-of-2, the code won't
mysteriously break.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:57: }
Nit: The old code was more readable than this more verbose version.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:69: // Done.
Nit: This comment adds nothing, remove it.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:74: dest += copy;
Nit: Can place this into the for loop declaration.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:75: dataPosition +=
copy;
Nit: Can omit this line and just pass (m_dataPosition + copy) to the function
call below.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:77: struct Cache {
I don't see what adding this struct buys other than more verbosity.

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReaderTest.cpp (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReaderTest.cpp:103: // of the
last segment works correctly.
Nit: How about:

Tests that a read from inside the penultimate segment to the very end of the
buffer doesn't try to read off the end of the buffer.

[kbalazs, 2015-03-25 21:05:19]

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:74: dest += copy;
On 2015/03/25 19:53:11, Peter Kasting wrote:
> Nit: Can place this into the for loop declaration.

copy is local to the loop

[Peter Kasting, 2015-03-25 21:10:48]

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.cpp (right):

https://codereview.chromium.org/1011113003/diff/20001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.cpp:74: dest += copy;
On 2015/03/25 21:05:19, kbalazs wrote:
> On 2015/03/25 19:53:11, Peter Kasting wrote:
> > Nit: Can place this into the for loop declaration.
> 
> copy is local to the loop

True.  Ignore that.

[kbalazs, 2015-03-26 19:05:29]

Comments incorporated, ptal.

[Peter Kasting, 2015-03-26 19:37:08]

LGTM

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:73: void
getSomeDataIntoCache(unsigned dataPosition);
Nit: getSomeDataInternal() might be a better name, since the functions of
getSomeData() and this are basically identical other than the return values, and
"IntoCache" isn't instantly clear to me as a reader.

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:82: // Data position in
|m_data| pointed by |segment|.
Nit: pointed -> pointed to

[kbalazs, 2015-03-26 20:33:11]

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
File Source/platform/image-decoders/FastSharedBufferReader.h (right):

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:73: void
getSomeDataIntoCache(unsigned dataPosition);
On 2015/03/26 19:37:08, Peter Kasting wrote:
> Nit: getSomeDataInternal() might be a better name, since the functions of
> getSomeData() and this are basically identical other than the return values,
and
> "IntoCache" isn't instantly clear to me as a reader.

Acknowledged.

https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
Source/platform/image-decoders/FastSharedBufferReader.h:82: // Data position in
|m_data| pointed by |segment|.
On 2015/03/26 19:37:08, Peter Kasting wrote:
> Nit: pointed -> pointed to

Ah, I didn't mean to change it. Originally it was saying "Data position in
|m_data| pointed by |m_segment|" which is correct since m_segment points into
m_data. I'm gonna change back.

[kbalazs, 2015-03-26 20:37:23]

The CQ bit was checked by b.kelemen@samsung.com

[kbalazs, 2015-03-26 20:37:24]

The patchset sent to the CQ was uploaded after l-g-t-m from hclam@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1011113003/#ps80001
(title: "last bits")

[commit-bot: I haz the power, 2015-03-26 20:37:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011113003/80001

[Peter Kasting, 2015-03-26 20:39:15]

On 2015/03/26 20:33:11, kbalazs wrote:
>
https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
> Source/platform/image-decoders/FastSharedBufferReader.h:82: // Data position
in
> |m_data| pointed by |segment|.
> On 2015/03/26 19:37:08, Peter Kasting wrote:
> > Nit: pointed -> pointed to
> 
> Ah, I didn't mean to change it. Originally it was saying "Data position in
> |m_data| pointed by |m_segment|" which is correct since m_segment points into
> m_data. I'm gonna change back.

You missed my point -- I wasn't objecting to "segment" (but good change there),
but to the fact that you dropped the preposition.  "pointed by" isn't
grammatical, you need to have "pointed to by".

[commit-bot: I haz the power, 2015-03-26 20:42:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-26 20:42:50]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[kbalazs, 2015-03-26 20:45:32]

On 2015/03/26 20:39:15, Peter Kasting wrote:
> On 2015/03/26 20:33:11, kbalazs wrote:
> >
>
https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
> > Source/platform/image-decoders/FastSharedBufferReader.h:82: // Data position
> in
> > |m_data| pointed by |segment|.
> > On 2015/03/26 19:37:08, Peter Kasting wrote:
> > > Nit: pointed -> pointed to
> > 
> > Ah, I didn't mean to change it. Originally it was saying "Data position in
> > |m_data| pointed by |m_segment|" which is correct since m_segment points
into
> > m_data. I'm gonna change back.
> 
> You missed my point -- I wasn't objecting to "segment" (but good change
there),
> but to the fact that you dropped the preposition.  "pointed by" isn't
> grammatical, you need to have "pointed to by".

I didn't drop anything, it was written that way at the first place. I'd prefer
not to change that myself since I'm anything but an expert of English grammar.

[Peter Kasting, 2015-03-26 20:46:52]

On 2015/03/26 20:45:32, kbalazs wrote:
> On 2015/03/26 20:39:15, Peter Kasting wrote:
> > On 2015/03/26 20:33:11, kbalazs wrote:
> > >
> >
>
https://codereview.chromium.org/1011113003/diff/60001/Source/platform/image-d...
> > > Source/platform/image-decoders/FastSharedBufferReader.h:82: // Data
position
> > in
> > > |m_data| pointed by |segment|.
> > > On 2015/03/26 19:37:08, Peter Kasting wrote:
> > > > Nit: pointed -> pointed to
> > > 
> > > Ah, I didn't mean to change it. Originally it was saying "Data position in
> > > |m_data| pointed by |m_segment|" which is correct since m_segment points
> into
> > > m_data. I'm gonna change back.
> > 
> > You missed my point -- I wasn't objecting to "segment" (but good change
> there),
> > but to the fact that you dropped the preposition.  "pointed by" isn't
> > grammatical, you need to have "pointed to by".
> 
> I didn't drop anything, it was written that way at the first place. I'd prefer
> not to change that myself since I'm anything but an expert of English grammar.

Fair enough, but please change it regardless.  (My grammar is good enough that
I'm quite confident in the correct form here.)

[pdr., 2015-03-27 20:53:18]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2015-03-27 20:53:18]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-03-27 20:53:19]

lgtm

[pdr., 2015-03-27 20:53:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, hclam@chromium.org
Link to the patchset: https://codereview.chromium.org/1011113003/#ps100001
(title: "grammar :)")

[commit-bot: I haz the power, 2015-03-27 20:53:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011113003/100001

[commit-bot: I haz the power, 2015-03-27 21:19:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 21:19:48]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[kbalazs, 2015-04-01 19:49:07]

The CQ bit was checked by b.kelemen@samsung.com

[kbalazs, 2015-04-01 19:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
pkasting@chromium.org, hclam@chromium.org
Link to the patchset: https://codereview.chromium.org/1011113003/#ps120001
(title: "more mess with the const...")

[commit-bot: I haz the power, 2015-04-01 19:50:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011113003/120001

[commit-bot: I haz the power, 2015-04-01 20:23:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-01 20:23:54]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[kbalazs, 2015-04-01 21:43:08]

Could you guys help me a bit with this freaky constant?

First I was thinking that a static const member variable can be initialized in
the header as the compiler will const-fold it thus no duplicated definition. It
works in gcc but clang doesn't like it.
Version 1:
 SharedBuffer.h:
  class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000; ...

I still wanted to keep it in the header to be const-folded (actually it only
used in the test so it doesn't really matter) so I figured I could initialize in
the header but also define it in the cpp.
 SharedBuffer.h:
  class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000; ...
 SharedBuffer.cpp:
  const unsigned SharedBuffer::kSegmentSize;

Clang and gcc is ok with this but MSVC is not.

So now I tried to just declare in the header and define with initialization in
the cpp but with that I need to heap allocate the arrays in the test which is
sad and also unrelated to the change.

Ah, I also tried using enum but than std::min(kSegmentSize, someUnsigned)
doesn't compile in C++11 it tries to use the override with
std::initializer_list.

Grepping through blink I can find examples for both version 1 and enum, so it
seems like those work sometime but not reliable.

Do you know a better way to do this, or should I just change the test? Maybe
something else?

Note that we cannot use constexpr as it is still black-listed, sadly.

[kbalazs, 2015-04-06 20:58:44]

I choose to refactor the test case. Please make a final rubber-stamp.

[pdr., 2015-04-07 23:19:11]

On 2015/04/06 at 20:58:44, b.kelemen wrote:
> I choose to refactor the test case. Please make a final rubber-stamp.

@pkasting, could you take another look at this? Looks like there are real
changes here.

[Peter Kasting, 2015-04-07 23:54:26]

On 2015/04/01 21:43:08, kbalazs wrote:
> Could you guys help me a bit with this freaky constant?
> 
> First I was thinking that a static const member variable can be initialized in
> the header as the compiler will const-fold it thus no duplicated definition.
It
> works in gcc but clang doesn't like it.
> Version 1:
>  SharedBuffer.h:
>   class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000;
...
> 
> I still wanted to keep it in the header to be const-folded (actually it only
> used in the test so it doesn't really matter) so I figured I could initialize
in
> the header but also define it in the cpp.
>  SharedBuffer.h:
>   class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000;
...
>  SharedBuffer.cpp:
>   const unsigned SharedBuffer::kSegmentSize;
> 
> Clang and gcc is ok with this but MSVC is not.

Sorry, I just now saw this.

First, if you use int instead of unsigned, I think version 1 might Just Work
everywhere, due to the way compilers special-case "int" for this.  (FWIW,
"unsigned" is probably not as correct as "size_t" here anyway... I don't really
like "unsigned" for this.)

Regardless, version 2 will definitely work everywhere if you simply prefix the
definition in the .cc file with STATIC_CONST_MEMBER_DEFINITION.  That does the
right magic to make MSVC stop complaining and is probably what I'd do.

Given those, you may want to reconsider how you decide to solve this.

[kbalazs, 2015-04-08 15:45:58]

On 2015/04/07 23:54:26, Peter Kasting wrote:
> On 2015/04/01 21:43:08, kbalazs wrote:
> > Could you guys help me a bit with this freaky constant?
> > 
> > First I was thinking that a static const member variable can be initialized
in
> > the header as the compiler will const-fold it thus no duplicated definition.
> It
> > works in gcc but clang doesn't like it.
> > Version 1:
> >  SharedBuffer.h:
> >   class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000;
> ...
> > 
> > I still wanted to keep it in the header to be const-folded (actually it only
> > used in the test so it doesn't really matter) so I figured I could
initialize
> in
> > the header but also define it in the cpp.
> >  SharedBuffer.h:
> >   class SharedBuffer { public: const static unsigned kSegmentSize = 0x1000;
> ...
> >  SharedBuffer.cpp:
> >   const unsigned SharedBuffer::kSegmentSize;
> > 
> > Clang and gcc is ok with this but MSVC is not.
> 
> Sorry, I just now saw this.
> 
> First, if you use int instead of unsigned, I think version 1 might Just Work
> everywhere, due to the way compilers special-case "int" for this.  (FWIW,
> "unsigned" is probably not as correct as "size_t" here anyway... I don't
really
> like "unsigned" for this.)
> 
> Regardless, version 2 will definitely work everywhere if you simply prefix the
> definition in the .cc file with STATIC_CONST_MEMBER_DEFINITION.  That does the
> right magic to make MSVC stop complaining and is probably what I'd do.
> 
> Given those, you may want to reconsider how you decide to solve this.

STATIC_CONST_MEMBER_DEFINITION is a chromium (base) thingy, there is no such
thing in blink land. :(
Afaik we still cannot use base from blink unfortunately. So, I would stick to
the latest version.

[Peter Kasting, 2015-04-08 21:02:09]

On 2015/04/08 15:45:58, kbalazs wrote:
> STATIC_CONST_MEMBER_DEFINITION is a chromium (base) thingy, there is no such
> thing in blink land. :(

Then define a copy on the Blink side.  It's a trivial macro definition.

[kbalazs, 2015-04-08 21:57:43]

On 2015/04/08 21:02:09, Peter Kasting wrote:
> On 2015/04/08 15:45:58, kbalazs wrote:
> > STATIC_CONST_MEMBER_DEFINITION is a chromium (base) thingy, there is no such
> > thing in blink land. :(
> 
> Then define a copy on the Blink side.  It's a trivial macro definition.

Done.

[Peter Kasting, 2015-04-08 22:02:50]

LGTM

[kbalazs, 2015-04-08 22:07:28]

The CQ bit was checked by b.kelemen@samsung.com

[kbalazs, 2015-04-08 22:07:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
hclam@chromium.org
Link to the patchset: https://codereview.chromium.org/1011113003/#ps180001
(title: "back to v6 plus define")

[commit-bot: I haz the power, 2015-04-08 22:08:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1011113003/180001

[commit-bot: I haz the power, 2015-04-09 00:20:16]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193404