Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
874683004:
Refactor the symbolize step of the orderfile generation. (Closed)
DescriptionRefactor the symbolize step of the orderfile generation.
This builds on the previous refactorings (linked in the bug), and unifies the
parsing of object files. It also removes the fuzzy matching of offsets done
that was previously also done in patch_orderfile.py.
It is also faster for three reasons:
- Elimination of an O(N^2) search
- Parallelization of object file parsing.
- No binary search
BUG=452879
Committed: https://crrev.com/753ce83612800633fe2081c139df118ef94d7c41
Cr-Commit-Position: refs/heads/master@{#314137}
Patch Set 1 #Patch Set 2 : Naming and typos. #
Total comments: 17
Patch Set 3 : Address comments. Change filenames. #
Total comments: 32
Patch Set 4 : Whitespace. #Patch Set 5 : Address comments. #Patch Set 6 : Address comments. #
Messages
Total messages: 15 (3 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
Hello, Here is another refactoring, this time for symbolize.py.
yay! please implement proper error handling via exit codes in this script, more details in inline comments https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:6: """Symbolize log file produced by cypgofile instrumentation. nit: cypgofile instrumentation (mergetraces.py) https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:8: Given a log file and the binary being profiled, create an order file. it's a misleading name for the file, since symbolization usually implies addr2line (i.e. the symbols and the files and the line numbers). How about cyglog_to_orderfile.py? trace_to_symbols? trace_to_orderfile? You can just create a new file in this CL, we will delete the old one later. To reduce the amount of human errors while using this script, let's introduce a mandatory output parameter where to write the unpatched orderfile. Create a temp file, rename to the original one in case there were no errors, and delete otherwise. Or even delete, and os.link('proc/self/fd/%d') on success? https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:22: """Parse a log file produced by the profiled run of chrome. nit: maybe enhance the comment with: (i.e. "merged cyglog") ? https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:39: call_info list with list of tuples of the format (sec, usec, call id, s/call_info list with// https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:45: assert "r-xp" in line s/"/'/ https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:103: raise SymbolNotFoundException(offset) If we raise an exception like this, it will be ignored by the calling script, and a half baked orderfile will be used, or even worse, the old orderfile. Please make sure main() returns 0 if we produced the orderfile successfully, and nonzero otherwise. This way in the next CL we will be able to *not* ignore this information. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:238: _WarnAboutDuplicates(offsets) how many warnings are produced in practice? https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:246: main() shouldn't we also set the logging level above this line in order to be consistent with patch_orderfile.py?
https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:6: """Symbolize log file produced by cypgofile instrumentation. On 2015/01/30 13:34:48, pasko wrote: > nit: cypgofile instrumentation (mergetraces.py) Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:8: Given a log file and the binary being profiled, create an order file. On 2015/01/30 13:34:48, pasko wrote: > it's a misleading name for the file, since symbolization usually implies > addr2line (i.e. the symbols and the files and the line numbers). > > How about cyglog_to_orderfile.py? trace_to_symbols? trace_to_orderfile? > > You can just create a new file in this CL, we will delete the old one later. > > To reduce the amount of human errors while using this script, let's introduce a > mandatory output parameter where to write the unpatched orderfile. Create a temp > file, rename to the original one in case there were no errors, and delete > otherwise. Or even delete, and os.link('proc/self/fd/%d') on success? Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:22: """Parse a log file produced by the profiled run of chrome. On 2015/01/30 13:34:48, pasko wrote: > nit: maybe enhance the comment with: > > (i.e. "merged cyglog") > > ? Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:39: call_info list with list of tuples of the format (sec, usec, call id, On 2015/01/30 13:34:48, pasko wrote: > s/call_info list with// Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:45: assert "r-xp" in line On 2015/01/30 13:34:48, pasko wrote: > s/"/'/ Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:103: raise SymbolNotFoundException(offset) On 2015/01/30 13:34:48, pasko wrote: > If we raise an exception like this, it will be ignored by the calling script, > and a half baked orderfile will be used, or even worse, the old orderfile. > > Please make sure main() returns 0 if we produced the orderfile successfully, and > nonzero otherwise. This way in the next CL we will be able to *not* ignore this > information. Done. https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:238: _WarnAboutDuplicates(offsets) On 2015/01/30 13:34:48, pasko wrote: > how many warnings are produced in practice? 0 +/- 0 (95% CL) https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:246: main() On 2015/01/30 13:34:48, pasko wrote: > shouldn't we also set the logging level above this line in order to be > consistent with patch_orderfile.py? Done.
On 2015/01/30 16:22:38, lizeb wrote: > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > File tools/cygprofile/symbolize.py (right): > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:6: """Symbolize log file produced by cypgofile > instrumentation. > On 2015/01/30 13:34:48, pasko wrote: > > nit: cypgofile instrumentation (mergetraces.py) > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:8: Given a log file and the binary being profiled, > create an order file. > On 2015/01/30 13:34:48, pasko wrote: > > it's a misleading name for the file, since symbolization usually implies > > addr2line (i.e. the symbols and the files and the line numbers). > > > > How about cyglog_to_orderfile.py? trace_to_symbols? trace_to_orderfile? > > > > You can just create a new file in this CL, we will delete the old one later. > > > > To reduce the amount of human errors while using this script, let's introduce > a > > mandatory output parameter where to write the unpatched orderfile. Create a > temp > > file, rename to the original one in case there were no errors, and delete > > otherwise. Or even delete, and os.link('proc/self/fd/%d') on success? > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:22: """Parse a log file produced by the profiled > run of chrome. > On 2015/01/30 13:34:48, pasko wrote: > > nit: maybe enhance the comment with: > > > > (i.e. "merged cyglog") > > > > ? > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:39: call_info list with list of tuples of the > format (sec, usec, call id, > On 2015/01/30 13:34:48, pasko wrote: > > s/call_info list with// > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:45: assert "r-xp" in line > On 2015/01/30 13:34:48, pasko wrote: > > s/"/'/ > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:103: raise SymbolNotFoundException(offset) > On 2015/01/30 13:34:48, pasko wrote: > > If we raise an exception like this, it will be ignored by the calling script, > > and a half baked orderfile will be used, or even worse, the old orderfile. > > > > Please make sure main() returns 0 if we produced the orderfile successfully, > and > > nonzero otherwise. This way in the next CL we will be able to *not* ignore > this > > information. > > Done. > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:238: _WarnAboutDuplicates(offsets) > On 2015/01/30 13:34:48, pasko wrote: > > how many warnings are produced in practice? > > 0 +/- 0 (95% CL) > > https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... > tools/cygprofile/symbolize.py:246: main() > On 2015/01/30 13:34:48, pasko wrote: > > shouldn't we also set the logging level above this line in order to be > > consistent with patch_orderfile.py? > > Done. Thank you for the review. PTAL.
https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/874683004/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:238: _WarnAboutDuplicates(offsets) On 2015/01/30 16:22:38, lizeb wrote: > On 2015/01/30 13:34:48, pasko wrote: > > how many warnings are produced in practice? > > 0 +/- 0 (95% CL) CI ? https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... File tools/cygprofile/cyglog_to_orderfile.py (right): https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:23: """Parse a merge cyglog produced by mergetraces.py. s/Parse/Parses/ (and similarly below) (sorry forgot to mention this in previous CLs) We try to follow the same practices in python as suggested in the C++ style: These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); the comment describes the function, it does not tell the function what to do. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... Similarly, the top-level description of the file should probably be: s/Symbolize/Symbolizes/ s/create/creates/ And finally here: s/merge/merged/ https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:51: call_lines.append(fields) else: assert fields[0] == 'END' wdyt? https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:56: (sec_timestamp, usec_timestamp) = map(int, call_line[0:2]) Why are we doing all this? We only use the ordered offsets later. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:81: def _FindSymbolInfosAtOffset(offset_to_symbol_infos, offset): not sure how strongly I feel about this. Usually the first argument would be 'what to find', and the second 'where to find'. Not sure why. Feel free to disregard. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:137: symbol_extractor.SymbolInfosFromBinary, object_filenames) maybe just write a comment saying that we expect everything to be in the page cache after linking, so I/O is _probably_ not a problem? https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:152: symbol_infos = [s for s in symbol_infos if not s.name.startswith('.LTHUNK')] I think it would be more readable if it lives in the loop: for symbol_info in symbol_infos: symbol = symbol_info.name if symbol.name.startswith('.LTHUNK'): continue; Higher order functions and lambda expressions are nice when they help making things clear, this line just made it a little more complex without clear reason. Also copied all of the array without a need, and provided an extra kick for GC. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:163: ' in incorrect section ' + section) I forgot why it would be incorrect for a symbol to be in a section ".text.Something", because .text is suppowed to be stripped? https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:192: """Output the order file to output_file. s/order file/orderfile/ https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:217: symbol_not_found_warnings.WriteEnd('did not find function') "X more warnings for did not find function" sounds awkward, maybe: "X more warnings for: function not found in the binary" (and also: symbol_not_found_warnings.Write('function not found in the binary')) https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:226: (log_file, lib_file, output_filename) = sys.argv[1:] nit: they are all file names, but two of them are _file, while one is _filename. log_filename and lib_filename would be more pleasing to my eye, but up to you https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:231: call_info = _ParseLogLines(log_file_lines) s/call_info/call_infos/ https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:245: symbol_to_section_map, output_file) nit: less whitespace https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:261: sys.exit(main()) so, symbol_extractor.CreateNameToSymbolInfo() is never used in this CL, is it going to be used in following CLs? If not, please delete it here with the corresponding unittest. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... File tools/cygprofile/cyglog_to_orderfile_unittest.py (right): https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile_unittest.py:9: import cyglog_to_orderfile should these be sorted alphabetically? (gotcha, my favorite nit) https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile_unittest.py:12: class TestSymbolizeTest(unittest.TestCase): nit: TestCyglogToOrderfile
https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... File tools/cygprofile/cyglog_to_orderfile.py (right): https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:23: """Parse a merge cyglog produced by mergetraces.py. On 2015/01/30 19:31:33, pasko wrote: > s/Parse/Parses/ (and similarly below) > (sorry forgot to mention this in previous CLs) We try to follow the same > practices in python as suggested in the C++ style: > > These comments should be descriptive ("Opens the file") rather than imperative > ("Open the file"); the comment describes the function, it does not tell the > function what to do. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > > Similarly, the top-level description of the file should probably be: > s/Symbolize/Symbolizes/ > s/create/creates/ > > And finally here: > s/merge/merged/ Ah, I was consistently doing the wrong thing. Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:51: call_lines.append(fields) On 2015/01/30 19:31:33, pasko wrote: > else: > assert fields[0] == 'END' > > wdyt? There are lines at the start that don't follow this pattern. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:56: (sec_timestamp, usec_timestamp) = map(int, call_line[0:2]) On 2015/01/30 19:31:33, pasko wrote: > Why are we doing all this? We only use the ordered offsets later. Carried over from the previous version of the code... Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:81: def _FindSymbolInfosAtOffset(offset_to_symbol_infos, offset): On 2015/01/30 19:31:33, pasko wrote: > not sure how strongly I feel about this. Usually the first argument would be > 'what to find', and the second 'where to find'. Not sure why. > > Feel free to disregard. Acknowledged. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:137: symbol_extractor.SymbolInfosFromBinary, object_filenames) On 2015/01/30 19:31:32, pasko wrote: > maybe just write a comment saying that we expect everything to be in the page > cache after linking, so I/O is _probably_ not a problem? Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:152: symbol_infos = [s for s in symbol_infos if not s.name.startswith('.LTHUNK')] On 2015/01/30 19:31:33, pasko wrote: > I think it would be more readable if it lives in the loop: > > for symbol_info in symbol_infos: > symbol = symbol_info.name > if symbol.name.startswith('.LTHUNK'): > continue; > > Higher order functions and lambda expressions are nice when they help making > things clear, this line just made it a little more complex without clear reason. > Also copied all of the array without a need, and provided an extra kick for GC. Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:163: ' in incorrect section ' + section) On 2015/01/30 19:31:32, pasko wrote: > I forgot why it would be incorrect for a symbol to be in a section > ".text.Something", because .text is suppowed to be stripped? It is the contrary, print a warning if the section names doesn't start with '.text'. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:192: """Output the order file to output_file. On 2015/01/30 19:31:33, pasko wrote: > s/order file/orderfile/ Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:217: symbol_not_found_warnings.WriteEnd('did not find function') On 2015/01/30 19:31:32, pasko wrote: > "X more warnings for did not find function" sounds awkward, maybe: > "X more warnings for: function not found in the binary" > (and also: symbol_not_found_warnings.Write('function not found in the binary')) Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:226: (log_file, lib_file, output_filename) = sys.argv[1:] On 2015/01/30 19:31:33, pasko wrote: > nit: they are all file names, but two of them are _file, while one is _filename. > > log_filename and lib_filename would be more pleasing to my eye, but up to you Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:231: call_info = _ParseLogLines(log_file_lines) On 2015/01/30 19:31:33, pasko wrote: > s/call_info/call_infos/ Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:245: symbol_to_section_map, output_file) On 2015/01/30 19:31:33, pasko wrote: > nit: less whitespace Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:261: sys.exit(main()) On 2015/01/30 19:31:32, pasko wrote: > so, symbol_extractor.CreateNameToSymbolInfo() is never used in this CL, is it > going to be used in following CLs? If not, please delete it here with the > corresponding unittest. Used later on. :-) https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... File tools/cygprofile/cyglog_to_orderfile_unittest.py (right): https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile_unittest.py:9: import cyglog_to_orderfile On 2015/01/30 19:31:33, pasko wrote: > should these be sorted alphabetically? (gotcha, my favorite nit) Done. https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile_unittest.py:12: class TestSymbolizeTest(unittest.TestCase): On 2015/01/30 19:31:33, pasko wrote: > nit: TestCyglogToOrderfile Done.
pasko@google.com changed reviewers: + pasko@google.com
OK, almost there, just a couple more questions https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... File tools/cygprofile/cyglog_to_orderfile.py (right): https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:51: call_lines.append(fields) On 2015/02/02 09:28:14, lizeb wrote: > On 2015/01/30 19:31:33, pasko wrote: > > else: > > assert fields[0] == 'END' > > > > wdyt? > > There are lines at the start that don't follow this pattern. But but .. you can replace the '2' with '3' in the 'log_file_lines[2:]', no? https://codereview.chromium.org/874683004/diff/40001/tools/cygprofile/cyglog_... tools/cygprofile/cyglog_to_orderfile.py:163: ' in incorrect section ' + section) On 2015/02/02 09:28:15, lizeb wrote: > On 2015/01/30 19:31:32, pasko wrote: > > I forgot why it would be incorrect for a symbol to be in a section > > ".text.Something", because .text is suppowed to be stripped? > > It is the contrary, print a warning if the section names doesn't start with > '.text'. ah, sorry, did not spot the 'not' in the condition, dumb. curious, cannotwait: how many warnings are printed in your experiments?
On Mon, Feb 2, 2015 at 11:35 AM, <pasko@google.com> wrote: > OK, almost there, just a couple more questions > > > https://codereview.chromium.org/874683004/diff/40001/ > tools/cygprofile/cyglog_to_orderfile.py > File tools/cygprofile/cyglog_to_orderfile.py (right): > > https://codereview.chromium.org/874683004/diff/40001/ > tools/cygprofile/cyglog_to_orderfile.py#newcode51 > tools/cygprofile/cyglog_to_orderfile.py:51: call_lines.append(fields) > On 2015/02/02 09:28:14, lizeb wrote: > >> On 2015/01/30 19:31:33, pasko wrote: >> > else: >> > assert fields[0] == 'END' >> > >> > wdyt? >> > > There are lines at the start that don't follow this pattern. >> > > But but .. you can replace the '2' with '3' in the 'log_file_lines[2:]', > no? Done. > > > https://codereview.chromium.org/874683004/diff/40001/ > tools/cygprofile/cyglog_to_orderfile.py#newcode163 > tools/cygprofile/cyglog_to_orderfile.py:163: ' in incorrect section ' + > section) > On 2015/02/02 09:28:15, lizeb wrote: > >> On 2015/01/30 19:31:32, pasko wrote: >> > I forgot why it would be incorrect for a symbol to be in a section >> > ".text.Something", because .text is suppowed to be stripped? >> > > It is the contrary, print a warning if the section names doesn't start >> > with > >> '.text'. >> > > ah, sorry, did not spot the 'not' in the condition, dumb. > > curious, cannotwait: how many warnings are printed in your experiments? > > 0. > https://codereview.chromium.org/874683004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm, thank you!
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874683004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/753ce83612800633fe2081c139df118ef94d7c41 Cr-Commit-Position: refs/heads/master@{#314137} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
