Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1660523003:
Complete unit tests for CPDF_Array (Closed)
DescriptionComplete unit tests for CPDF_Array
Also remove one unnecessary member function.
R=thestig@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/6f358daa2e8b1639a1c3a3f07a449f8450197e8b
Patch Set 1 : #
Total comments: 27
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Address more comments #
Messages
Total messages: 12 (5 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Complete unit tests for CPDF_Array ========== to ========== Complete unit tests for CPDF_Array Also remove one unnecessary member function. ==========
weili@chromium.org changed reviewers: + thestig@chromium.org
PTAL, thanks
https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, Does it need to be inline? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, Maybe TestArrayAccessors() ? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, const CPDF_Array* since all the Get methods are const. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:26: EXPECT_STREQ(arr->GetStringAt(index).c_str(), str_val); Note: gtest functions are in form of EXPECT_EQ(expected, actual), and you've written it the other way around in many places. This becomes confusing when tests fail. For example: int print_answer() { return 40; } EXPECT_EQ(42, print_answer()); will cause gtest to print out: test failed, expected: 40, got: 42. When it really should have printed: test failed, expected: 42, got: 40. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:467: std::to_string(vals[i]).c_str(), // String value. This is currently on the list of C++11 features to be discussed, so too early to start using it. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:468: nullptr, // Const string value. Huh, I'm surprised there is a string value, but not a const string value. But if that's how it is, then ok. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:501: ScopedArray arr(new CPDF_Array); Maybe arr -> string_array and arr1 -> name_array ? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:635: CPDF_Array* arr_val = new CPDF_Array; InsertAt() takes ownership, right? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:648: uint8_t* buf = reinterpret_cast<uint8_t*>(malloc(buf_size)); Does the CPDF_Stream ctor take ownership? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:662: for (size_t i = 0; i < 14; ++i) { 14 -> FX_ArraySize(expected_str) ? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:679: EXPECT_EQ(arr->GetStreamAt(i), stream_val); Combine with the other i == 13 block above? https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:729: CPDF_Boolean* boolean_obj = new CPDF_Boolean(true); Who takes ownership of these objects? InsertIndirectObject() ?
https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, On 2016/02/02 23:00:38, Lei Zhang wrote: > Maybe TestArrayAccessors() ? Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, On 2016/02/02 23:00:38, Lei Zhang wrote: > const CPDF_Array* since all the Get methods are const. Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:17: inline void TestAccessors(CPDF_Array* arr, On 2016/02/02 23:00:38, Lei Zhang wrote: > Does it need to be inline? Probably doesn't matter much. Removed. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:26: EXPECT_STREQ(arr->GetStringAt(index).c_str(), str_val); On 2016/02/02 23:00:38, Lei Zhang wrote: > Note: gtest functions are in form of EXPECT_EQ(expected, actual), and you've > written it the other way around in many places. This becomes confusing when > tests fail. For example: > > int print_answer() { return 40; } > EXPECT_EQ(42, print_answer()); > > will cause gtest to print out: > > test failed, expected: 40, got: 42. > > When it really should have printed: > > test failed, expected: 42, got: 40. Thanks for the elaboration. Now changed them all. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:467: std::to_string(vals[i]).c_str(), // String value. On 2016/02/02 23:00:38, Lei Zhang wrote: > This is currently on the list of C++11 features to be discussed, so too early to > start using it. Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:468: nullptr, // Const string value. On 2016/02/02 23:00:38, Lei Zhang wrote: > Huh, I'm surprised there is a string value, but not a const string value. But if > that's how it is, then ok. GetConstString returns the string buffer pointer inside the object, which integer objects don't have. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:501: ScopedArray arr(new CPDF_Array); On 2016/02/02 23:00:38, Lei Zhang wrote: > Maybe arr -> string_array and arr1 -> name_array ? Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:635: CPDF_Array* arr_val = new CPDF_Array; On 2016/02/02 23:00:38, Lei Zhang wrote: > InsertAt() takes ownership, right? Yes. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:648: uint8_t* buf = reinterpret_cast<uint8_t*>(malloc(buf_size)); On 2016/02/02 23:00:38, Lei Zhang wrote: > Does the CPDF_Stream ctor take ownership? Yes. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:662: for (size_t i = 0; i < 14; ++i) { On 2016/02/02 23:00:38, Lei Zhang wrote: > 14 -> FX_ArraySize(expected_str) ? Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:679: EXPECT_EQ(arr->GetStreamAt(i), stream_val); On 2016/02/02 23:00:38, Lei Zhang wrote: > Combine with the other i == 13 block above? Done. https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:729: CPDF_Boolean* boolean_obj = new CPDF_Boolean(true); On 2016/02/02 23:00:38, Lei Zhang wrote: > Who takes ownership of these objects? InsertIndirectObject() ? Yes, holder (class CPDF_IndirectObjectHolder) will be in charge of freeing them.
https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:501: ScopedArray arr(new CPDF_Array); On 2016/02/03 01:05:48, Wei Li wrote: > On 2016/02/02 23:00:38, Lei Zhang wrote: > > Maybe arr -> string_array and arr1 -> name_array ? > > Done. Not done here. :-P https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:635: CPDF_Array* arr_val = new CPDF_Array; On 2016/02/03 01:05:48, Wei Li wrote: > On 2016/02/02 23:00:38, Lei Zhang wrote: > > InsertAt() takes ownership, right? > > Yes. It would be good to add a comment in these cases, as this is confusing for a reader unless they are familiar with the code. Maybe one day we'll change our code to have unique_ptrs everywhere to signify ownership. https://codereview.chromium.org/1660523003/diff/30001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/30001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:576: key.append(buf); Just: key.append(FXSYS_itoa(j, buf, 10)); Ditto below.
(hit the wrong button) lgtm
Patchset #3 (id:50001) has been deleted
thanks! https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/2/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:501: ScopedArray arr(new CPDF_Array); On 2016/02/03 01:19:17, Lei Zhang wrote: > On 2016/02/03 01:05:48, Wei Li wrote: > > On 2016/02/02 23:00:38, Lei Zhang wrote: > > > Maybe arr -> string_array and arr1 -> name_array ? > > > > Done. > > Not done here. :-P Oops, fixed another place. :) Done. https://codereview.chromium.org/1660523003/diff/30001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right): https://codereview.chromium.org/1660523003/diff/30001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:576: key.append(buf); On 2016/02/03 01:19:17, Lei Zhang wrote: > Just: key.append(FXSYS_itoa(j, buf, 10)); > > Ditto below. Done.
Description was changed from ========== Complete unit tests for CPDF_Array Also remove one unnecessary member function. ========== to ========== Complete unit tests for CPDF_Array Also remove one unnecessary member function. R=thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/6f358daa2e8b1639a1c3a3f07a449f845019... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:70001) manually as 6f358daa2e8b1639a1c3a3f07a449f8450197e8b (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
