Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421293008:
include what you use: errno.h in exec_process.cc (Closed)
|
Created:
5 years, 1 month ago by Mostyn Bramley-Moore Modified:
5 years, 1 month ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioninclude what you use: errno.h in exec_process.cc
Add missing errno.h inclusion - this is less likely to
break uncommon toolchains (eg uClibc). And while we're
at it, merge posix includes into else case for an
#ifdef OS_WIN block to match the code that uses these
includes.
R=brettw@chromium.org,tfarina@chromium.org
Committed: https://crrev.com/9eca8d034b9a15b634e45d1ef3dde4306ae6a355
Cr-Commit-Position: refs/heads/master@{#358944}
Patch Set 1 #
Total comments: 6
Patch Set 2 : merge posix includes into else case #
Total comments: 1
Messages
Total messages: 21 (7 generated)
mostynb@opera.com changed reviewers: + brettw@chromium.org
@brettw: here's an include-what-you-use fixup.
tfarina@chromium.org changed reviewers: + tfarina@chromium.org
Mostyn, could you add to the CL description the toolchain you have used that encountered this problem? I would probably include the error too.
On 2015/11/07 10:50:55, tfarina wrote: > Mostyn, could you add to the CL description the toolchain you have used that > encountered this problem? I would probably include the error too. I did not encounter an error in this case. This is a preemptive fix/generally good practice.
I think this is correct, I see usages of errno in this file. But I was mainly curious on how you get to this one off. Did you happen to be reading this file and noticed it? https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> Maybe do this in the POSIX case below?
> I think this is correct, I see usages of errno in this file. But I was mainly > curious on how you get to this one off. Did you happen to be reading this file > and noticed it? We encountered actual breakage elsewhere (two separate instances) in the tree recently due to missing errno.h includes with uClibc based toolchains. So I did a semi-manual check for the entire tree, based on the fact that "errno" is both the name of the the variable and part of the the required header filename: ie "git grep errno" and scan through the results- for each file that matches, the first line should be an errno.h include. Sorry this isn't a general solution :) https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> On 2015/11/07 16:10:56, tfarina wrote: > Maybe do this in the POSIX case below? In this file errno is only used inside the else block of an OS_WIN condition, so I think this is possibly (very) slightly more accurate. But this is personal preference, really. Maybe we should just include errno.h unconditionally- I believe this is safe.
OK, I think you should include the uClibc information in the CL description. This the vital information for us understand why this change was made in future.
https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> On 2015/11/07 19:52:39, Mostyn Bramley-Moore wrote: > On 2015/11/07 16:10:56, tfarina wrote: > > Maybe do this in the POSIX case below? > > In this file errno is only used inside the else block of an OS_WIN condition, so > I think this is possibly (very) slightly more accurate. Then should we move the includes below in this else instead?
Description was changed from ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion. ========== to ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion- this is less likely to break uncommon toolchains (eg uClibc). ==========
Adjusted the description a little. https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> On 2015/11/08 13:59:33, tfarina wrote: > On 2015/11/07 19:52:39, Mostyn Bramley-Moore wrote: > > On 2015/11/07 16:10:56, tfarina wrote: > > > Maybe do this in the POSIX case below? > > > > In this file errno is only used inside the else block of an OS_WIN condition, > so > > I think this is possibly (very) slightly more accurate. > Then should we move the includes below in this else instead? I will defer to Brett on this.
lgtm https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> Personally I'd merge them all into the else.
Description was changed from ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion- this is less likely to break uncommon toolchains (eg uClibc). ========== to ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion- this is less likely to break uncommon toolchains (eg uClibc). And while we're at it, merge posix includes into else case for an #ifdef OS_WIN block to match the code that uses these includes. ==========
https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/1/tools/gn/exec_process.cc#ne... tools/gn/exec_process.cc:18: #include <errno.h> On 2015/11/10 21:48:19, brettw wrote: > Personally I'd merge them all into the else. Done.
The CQ bit was checked by mostynb@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1421293008/#ps20001 (title: "merge posix includes into else case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421293008/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421293008/20001
lgtm. https://codereview.chromium.org/1421293008/diff/20001/tools/gn/exec_process.cc File tools/gn/exec_process.cc (right): https://codereview.chromium.org/1421293008/diff/20001/tools/gn/exec_process.c... tools/gn/exec_process.cc:4: just noticed this: exec_process.cc isn't including exec_process.h. I will send a separate CL to fix this.
Description was changed from ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion- this is less likely to break uncommon toolchains (eg uClibc). And while we're at it, merge posix includes into else case for an #ifdef OS_WIN block to match the code that uses these includes. ========== to ========== include what you use: errno.h in exec_process.cc Add missing errno.h inclusion - this is less likely to break uncommon toolchains (eg uClibc). And while we're at it, merge posix includes into else case for an #ifdef OS_WIN block to match the code that uses these includes. R=brettw@chromium.org,tfarina@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9eca8d034b9a15b634e45d1ef3dde4306ae6a355 Cr-Commit-Position: refs/heads/master@{#358944} |
