Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1467713004:
initializing ICU in url fuzzer. (Closed)
Descriptioninitializing ICU in url fuzzer.
BUG=
Committed: https://crrev.com/b88ffb2d8d3188ad74f8e6585efc77d41827a1a5
Cr-Commit-Position: refs/heads/master@{#361011}
Patch Set 1 #
Total comments: 3
Patch Set 2 : review #
Total comments: 4
Patch Set 3 : review #
Messages
Total messages: 24 (8 generated)
aizatsky@chromium.org changed reviewers: + jshin@chromium.org, krasin@chromium.org
krasin@google.com changed reviewers: + krasin@google.com
https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase test_case; Per Google C++ style guide ([1]), which Chromium follows with a few exceptions ([2]), """Objects with static storage duration, including global variables, static variables, static class member variables, and function static variables, must be Plain Old Data (POD): only ints, chars, floats, or pointers, or arrays/structs of POD.""" Please, define a main function for this fuzzer and put this initialization into main. 1. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables 2. https://www.chromium.org/developers/coding-style
https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase test_case; On 2015/11/21 00:07:12, krasin wrote: > Per Google C++ style guide ([1]), which Chromium follows with a few exceptions > ([2]), > > """Objects with static storage duration, including global variables, static > variables, static class member variables, and function static variables, must be > Plain Old Data (POD): only ints, chars, floats, or pointers, or arrays/structs > of POD.""" > > Please, define a main function for this fuzzer and put this initialization into > main. > > 1. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > 2. https://www.chromium.org/developers/coding-style This is the exact opposite of what Kostya asks to do (see V8 fuzzer);
On 2015/11/21 00:11:36, aizatsky wrote: > https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... > File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): > > https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... > testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase test_case; > On 2015/11/21 00:07:12, krasin wrote: > > Per Google C++ style guide ([1]), which Chromium follows with a few exceptions > > ([2]), > > > > """Objects with static storage duration, including global variables, static > > variables, static class member variables, and function static variables, must > be > > Plain Old Data (POD): only ints, chars, floats, or pointers, or arrays/structs > > of POD.""" > > > > Please, define a main function for this fuzzer and put this initialization > into > > main. > > > > 1. > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > 2. https://www.chromium.org/developers/coding-style > > This is the exact opposite of what Kostya asks to do (see V8 fuzzer); V8 fuzzer (https://code.google.com/p/chromium/codesearch#chromium/src/testing/libfuzzer/...) has a static variable of a pointer type: static Isolate* isolate = Init(); There's nothing wrong with that, but the destructor won't be called. In the current CL, the type of the static variable is a struct that includes fields of non-POD types, which violates the style guide.
https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): https://codereview.chromium.org/1467713004/diff/1/testing/libfuzzer/fuzzers/u... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase test_case; On 2015/11/21 00:11:36, aizatsky wrote: > On 2015/11/21 00:07:12, krasin wrote: > > Per Google C++ style guide ([1]), which Chromium follows with a few exceptions > > ([2]), > > > > """Objects with static storage duration, including global variables, static > > variables, static class member variables, and function static variables, must > be > > Plain Old Data (POD): only ints, chars, floats, or pointers, or arrays/structs > > of POD.""" > > > > Please, define a main function for this fuzzer and put this initialization > into > > main. > > > > 1. > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > > > 2. https://www.chromium.org/developers/coding-style > > This is the exact opposite of what Kostya asks to do (see V8 fuzzer); replaced by a pointer.
> replaced by a pointer. Thanks. Note that at_exit_manager will never actually fire, but we don't probably care about this for fuzzing. LGTM
lgtm
aizatsky@chromium.org changed reviewers: + inferno@chromium.org
aarya@google.com changed reviewers: + aarya@google.com
lgtm with nits. https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:11: assert(base::i18n::InitializeICU()); We usually use CHECK() macros for these. https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase *test_case = new TestCase(); nit: * on the left side.
https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... File testing/libfuzzer/fuzzers/url_parse_fuzzer.cc (right): https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:11: assert(base::i18n::InitializeICU()); On 2015/11/21 05:23:03, aarya wrote: > We usually use CHECK() macros for these. Done. https://codereview.chromium.org/1467713004/diff/20001/testing/libfuzzer/fuzze... testing/libfuzzer/fuzzers/url_parse_fuzzer.cc:18: TestCase *test_case = new TestCase(); On 2015/11/21 05:23:03, aarya wrote: > nit: * on the left side. Done.
The CQ bit was checked by aizatsky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from krasin@chromium.org, krasin@google.com, aarya@google.com Link to the patchset: https://codereview.chromium.org/1467713004/#ps40001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467713004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467713004/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by inferno@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467713004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467713004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b88ffb2d8d3188ad74f8e6585efc77d41827a1a5 Cr-Commit-Position: refs/heads/master@{#361011} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
