BSD port changes for base/ (OS_POSIX/GTK instead of OS_LINUX...

BSD port changes for base/ (OS_POSIX/GTK instead of OS_LINUX
where applicable, missing includes, etc)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41143

[Evan Martin, 2010-03-09 23:24:57]

http://codereview.chromium.org/774001/diff/9001/10005
File base/file_util_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10005#newcode42
base/file_util_posix.cc:42: #if defined(OS_OPENBSD) || defined(OS_FREEBSD) || \
no 64-bit stat?  :~(

http://codereview.chromium.org/774001/diff/9001/10007
File base/linux_util.cc (right):

http://codereview.chromium.org/774001/diff/9001/10007#newcode234
base/linux_util.cc:234: #endif
This shouldn't compile, since it never returns.


...hey, wait a sec, do you even build linux_util on openbsd?  A bit weird...

http://codereview.chromium.org/774001/diff/9001/10002
File base/process_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10002#newcode9
base/process_posix.cc:9: #include "base/process.h"
Can you make this the topmost include?  with a newline after, i.e. matching
Google C++ style.

http://codereview.chromium.org/774001/diff/9001/10004
File base/process_util.h (right):

http://codereview.chromium.org/774001/diff/9001/10004#newcode481
base/process_util.h:481: #if defined(OS_POSIX) && !defined(OS_MACOSX)
maybe reoder after OS_MACOSX here instead like you do in the .cc?

http://codereview.chromium.org/774001/diff/9001/10001
File base/process_util_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10001#newcode272
base/process_util_posix.cc:272: #elif defined(OS_MACOSX) || defined(OS_OPENBSD)
|| defined(OS_FREEBSD) || \
Maybe alphabetize after FBSD for grepping's sake?

http://codereview.chromium.org/774001/diff/9001/10003
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/774001/diff/9001/10003#newcode9
base/sys_info_openbsd.cc:9: #include <sys/param.h>
These go above base/logging.h  (Google c++ style)

[Peter Valchev, 2010-03-09 23:39:11]

http://codereview.chromium.org/774001/diff/9001/10005
File base/file_util_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10005#newcode42
base/file_util_posix.cc:42: #if defined(OS_OPENBSD) || defined(OS_FREEBSD) || \
On 2010/03/09 23:24:57, Evan Martin wrote:
> no 64-bit stat?  :~(

the stat(2) syscall on BSD (or more precisely the struct stat) handles 64-bit
files etc. no problem.

i think stat64 is a linuxism because the original stat structure was using the
wrong types perhaps, but i could be misremembering the details...

http://codereview.chromium.org/774001/diff/9001/10007
File base/linux_util.cc (right):

http://codereview.chromium.org/774001/diff/9001/10007#newcode234
base/linux_util.cc:234: #endif
On 2010/03/09 23:24:57, Evan Martin wrote:
> This shouldn't compile, since it never returns.
> 
> 
> ...hey, wait a sec, do you even build linux_util on openbsd?  A bit weird...

Yes, we do build linux_util and most other *linux* files, i know it is weird, it
came up a couple of times before too :)

I think ideally these files will be renamed to *posix* etc. as I don't think it
makes sense to copy/paste this code to a 'bsd' file and then remove the
linuxisms... but that's not super clean either. Sigh.

http://codereview.chromium.org/774001/diff/9001/10002
File base/process_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10002#newcode9
base/process_posix.cc:9: #include "base/process.h"
On 2010/03/09 23:24:57, Evan Martin wrote:
> Can you make this the topmost include?  with a newline after, i.e. matching
> Google C++ style.

Done. It's not process_posix.h so that's why I didn't notice this, but I guess
process.h is used by all of the *process* files

http://codereview.chromium.org/774001/diff/9001/10004
File base/process_util.h (right):

http://codereview.chromium.org/774001/diff/9001/10004#newcode481
base/process_util.h:481: #if defined(OS_POSIX) && !defined(OS_MACOSX)
On 2010/03/09 23:24:57, Evan Martin wrote:
> maybe reoder after OS_MACOSX here instead like you do in the .cc?

Done.

http://codereview.chromium.org/774001/diff/9001/10001
File base/process_util_posix.cc (right):

http://codereview.chromium.org/774001/diff/9001/10001#newcode272
base/process_util_posix.cc:272: #elif defined(OS_MACOSX) || defined(OS_OPENBSD)
|| defined(OS_FREEBSD) || \
On 2010/03/09 23:24:57, Evan Martin wrote:
> Maybe alphabetize after FBSD for grepping's sake?

Done.

http://codereview.chromium.org/774001/diff/9001/10003
File base/sys_info_openbsd.cc (right):

http://codereview.chromium.org/774001/diff/9001/10003#newcode9
base/sys_info_openbsd.cc:9: #include <sys/param.h>
On 2010/03/09 23:24:57, Evan Martin wrote:
> These go above base/logging.h  (Google c++ style)

Done.

[Evan Martin, 2010-03-09 23:50:29]

http://codereview.chromium.org/774001/diff/18001/19007
File base/linux_util.cc (right):

http://codereview.chromium.org/774001/diff/18001/19007#newcode204
base/linux_util.cc:204: #elif defined(OS_LINUX)
I still don't think this compiles, because you never return anything in the else
case.  Does it compile?

[Peter Valchev, 2010-03-09 23:55:57]

http://codereview.chromium.org/774001/diff/18001/19007
File base/linux_util.cc (right):

http://codereview.chromium.org/774001/diff/18001/19007#newcode204
base/linux_util.cc:204: #elif defined(OS_LINUX)
On 2010/03/09 23:50:29, Evan Martin wrote:
> I still don't think this compiles, because you never return anything in the
else
> case.  Does it compile?

Yes, it compiles & runs (runs on FreeBSD... on OpenBSD I'm debugging some
utf8-related crash, otherwise I have synced up my patches to trunk but there's
some new breakage :()

OK I just checked my build log and it does show a warning, and we don't use
-Werror on *BSD

base/linux_util.cc: In function 'std::string base::GetLinuxDistro()':^M
base/linux_util.cc:235: warning: control reaches end of non-void function^M

How about I just modify it to return "" ? But I do know this is all hackish. So
I'd say we should leave it as-is for now since it doesn't affect
Linux/MacOS/Windows and doesn't make the code any uglier than it is.

Long term I do think we should clean stuff like this up, but so much to do :(

[Evan Martin, 2010-03-09 23:59:06]

On 2010/03/09 23:55:57, Peter Valchev wrote:
> OK I just checked my build log and it does show a warning, and we don't use
> -Werror on *BSD
> 
> base/linux_util.cc: In function 'std::string base::GetLinuxDistro()':^M
> base/linux_util.cc:235: warning: control reaches end of non-void function^M
> 
> How about I just modify it to return "" ? But I do know this is all hackish.
So
> I'd say we should leave it as-is for now since it doesn't affect
> Linux/MacOS/Windows and doesn't make the code any uglier than it is.

That seems ok.  You can also do #else NOTIMPLEMENTED(), which prints out a
warning message with a line number at runtime.  I worry that not returning
anything may cause a crash or something, but maybe C++ is clever enough to use
the default ctor or something.

> Long term I do think we should clean stuff like this up, but so much to do :(

Understood.

LGTM

[Peter Valchev, 2010-03-10 07:48:30]

On 2010/03/09 23:59:06, Evan Martin wrote:
> On 2010/03/09 23:55:57, Peter Valchev wrote:
> > OK I just checked my build log and it does show a warning, and we don't use
> > -Werror on *BSD
> > 
> > base/linux_util.cc: In function 'std::string base::GetLinuxDistro()':^M
> > base/linux_util.cc:235: warning: control reaches end of non-void function^M
> > 
> > How about I just modify it to return "" ? But I do know this is all hackish.
> So
> > I'd say we should leave it as-is for now since it doesn't affect
> > Linux/MacOS/Windows and doesn't make the code any uglier than it is.
> 
> That seems ok.  You can also do #else NOTIMPLEMENTED(), which prints out a
> warning message with a line number at runtime.  I worry that not returning
> anything may cause a crash or something, but maybe C++ is clever enough to use
> the default ctor or something.

I added the NOTIMPLEMENTED() and committed it. Thanks so much!

> 
> > Long term I do think we should clean stuff like this up, but so much to do
:(
> 
> Understood.
> 
> LGTM