Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(668)

Issue 411002: Remove some linux libc calls from client library. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 8 months ago by digit
Modified:
11 years, 8 months ago
CC:
google-breakpad-dev_googlegroups.com
Base URL:
http://google-breakpad.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Remove some linux libc calls from client library.

This patch removes a few libc calls from the client library
when generating a minidump.

+ Move the static inlined functions from linux_libc_support.h
  into their own source file to avoid code duplication.

Tested on linux-x86_64 with 'make check' and 'android/run-checks.sh'
Committed: https://code.google.com/p/google-breakpad/source/detail?r=1001

Patch Set 1 #

Patch Set 2 : #

Total comments: 4

Patch Set 3 : #

Total comments: 4

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M Makefile.am View 1 2 4 chunks +8 lines, -4 lines 0 comments Download
M Makefile.in View 1 2 20 chunks +112 lines, -77 lines 0 comments Download
M src/client/linux/minidump_writer/directory_reader.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/line_reader.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M src/client/linux/minidump_writer/linux_dumper.cc View 1 2 5 chunks +8 lines, -8 lines 0 comments Download
M src/client/linux/minidump_writer/linux_ptrace_dumper.cc View 1 2 4 chunks +8 lines, -8 lines 0 comments Download
M src/client/linux/minidump_writer/minidump_writer.cc View 1 2 22 chunks +47 lines, -45 lines 0 comments Download
A + src/common/linux/linux_libc_support.cc View 1 2 3 8 chunks +79 lines, -29 lines 0 comments Download
M src/common/linux/linux_libc_support.h View 1 2 1 chunk +23 lines, -106 lines 0 comments Download
M src/common/linux/linux_libc_support_unittest.cc View 1 2 2 chunks +38 lines, -0 lines 0 comments Download

Messages

Total messages: 11
digit
A few remarks about this patch: - I tried to follow the coding style of ...
11 years, 8 months ago #1
Mark Mentovai
I don’t know how I feel about all of these inlines. Except for the strl ...
11 years, 8 months ago #2
Mark Mentovai
(Android doesn’t need to subscribe to my proposed default of using the standard library.)
11 years, 8 months ago #3
digit
I've rewritten the patch to suit your recommendations. Let me know what you think about ...
11 years, 8 months ago #4
Mark Mentovai
LGTM http://breakpad.appspot.com/411002/diff/4001/src/common/linux/linux_libc_support.h File src/common/linux/linux_libc_support.h (right): http://breakpad.appspot.com/411002/diff/4001/src/common/linux/linux_libc_support.h#newcode142 src/common/linux/linux_libc_support.h:142: static inline const char* I’m still concerned about ...
11 years, 8 months ago #5
Ted Mielczarek
https://breakpad.appspot.com/411002/diff/4001/src/client/linux/minidump_writer/linux_dumper.cc File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/411002/diff/4001/src/client/linux/minidump_writer/linux_dumper.cc#newcode158 src/client/linux/minidump_writer/linux_dumper.cc:158: close(fd); sys_close here too.
11 years, 8 months ago #6
digit
https://breakpad.appspot.com/411002/diff/4001/src/client/linux/minidump_writer/linux_dumper.cc File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/411002/diff/4001/src/client/linux/minidump_writer/linux_dumper.cc#newcode158 src/client/linux/minidump_writer/linux_dumper.cc:158: close(fd); thanks, will fix. https://breakpad.appspot.com/411002/diff/4001/src/common/linux/linux_libc_support.h File src/common/linux/linux_libc_support.h (right): https://breakpad.appspot.com/411002/diff/4001/src/common/linux/linux_libc_support.h#newcode142 ...
11 years, 8 months ago #7
digit
Patch set 3 contains the fixes described above. The new source file is src/ommon/linux/linux_libc_support.cc
11 years, 8 months ago #8
Mark Mentovai
https://breakpad.appspot.com/411002/diff/10001/src/common/linux/linux_libc_support.cc File src/common/linux/linux_libc_support.cc (right): https://breakpad.appspot.com/411002/diff/10001/src/common/linux/linux_libc_support.cc#newcode1 src/common/linux/linux_libc_support.cc:1: // Copyright (c) 2012, Google Inc. If you’re using ...
11 years, 8 months ago #9
digit
https://breakpad.appspot.com/411002/diff/10001/src/common/linux/linux_libc_support.cc File src/common/linux/linux_libc_support.cc (right): https://breakpad.appspot.com/411002/diff/10001/src/common/linux/linux_libc_support.cc#newcode1 src/common/linux/linux_libc_support.cc:1: // Copyright (c) 2012, Google Inc. will do https://breakpad.appspot.com/411002/diff/10001/src/common/linux/linux_libc_support.cc#newcode40 ...
11 years, 8 months ago #10
Mark Mentovai
11 years, 8 months ago #11
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1004:630ec63f810e-tainted