Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8991010:
This change updates the libjingle logging so that it writes to chromiums VLOG instead of its own ... (Closed)
|
Created:
8 years, 12 months ago by hellner Modified:
8 years, 11 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, Paweł Hajdan Jr., sergeyu+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionThis change updates the libjingle logging so that it writes to chromiums VLOG instead of its own logging mechanism.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117917
Patch Set 1 #
Total comments: 12
Patch Set 2 : '' #
Total comments: 58
Patch Set 3 : '' #
Total comments: 12
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 6
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #
Total comments: 4
Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #
Total comments: 2
Patch Set 15 : '' #
Messages
Total messages: 16 (0 generated)
The change overrides the libjingle logging macros. Please let me know if anything is unclear and I'll elaborate. Please have a go at reviewing this CL. Note, I won't be back until January 9th with limited internet access so there is no hurry in reviewing this (once I'm back we do want to get this in ASAP though).
It's great to see this being fixed! Please see my comments. http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc File jingle/glue/logging_unittest.cc (right): http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc... jingle/glue/logging_unittest.cc:5: // Note: this test only tests LOG_V and LOG_E since all other logs are I think it't worth testing LOG() too. http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc... jingle/glue/logging_unittest.cc:10: // This file must be included first to ensure that libjingle style logging is This is required by code style anyway, so this comment is confusing. I think it's more reliable to just define LOGGING_INSIDE_LIBJINGLE here (with comments). http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... File third_party/libjingle/libjingle.gyp (right): http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:17: 'SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS', Remove this? http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:152: # SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS to work. Update this comment. http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:153: # TODO(sergeyu): push SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS to this TODO is no longer relevant. Remove it? http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/overrides... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/overrides... third_party/libjingle/overrides/talk/base/logging.h:32: #define LOGGING_INSIDE_LIBJINGLE Can we define this in the gyp in the same way SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS was defined there?
Updated according to comments. Please have another go at reviewing. Thanks! http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc File jingle/glue/logging_unittest.cc (right): http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc... jingle/glue/logging_unittest.cc:5: // Note: this test only tests LOG_V and LOG_E since all other logs are On 2011/12/28 02:38:15, sergeyu wrote: > I think it't worth testing LOG() too. It's basically the same as LOG_V it only prepends talk_base:: so you don't have to type it. If this fails it should do so compile time. However, I did add one test case for it. http://codereview.chromium.org/8991010/diff/1/jingle/glue/logging_unittest.cc... jingle/glue/logging_unittest.cc:10: // This file must be included first to ensure that libjingle style logging is On 2011/12/28 02:38:15, sergeyu wrote: > This is required by code style anyway, so this comment is confusing. > I think it's more reliable to just define LOGGING_INSIDE_LIBJINGLE here (with > comments). Done. http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... File third_party/libjingle/libjingle.gyp (right): http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:17: 'SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS', On 2011/12/28 02:38:15, sergeyu wrote: > Remove this? I looked through the chromium code and there doesn't seem to be any conflicts with the macros this define enables. Also this define is only used in libjingle's logging.h but not in this override so we should be able to remove this. Done. http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:152: # SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS to work. On 2011/12/28 02:38:15, sergeyu wrote: > Update this comment. Done. http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/libjingle... third_party/libjingle/libjingle.gyp:153: # TODO(sergeyu): push SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS to On 2011/12/28 02:38:15, sergeyu wrote: > this TODO is no longer relevant. Remove it? Done. http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/overrides... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/1/third_party/libjingle/overrides... third_party/libjingle/overrides/talk/base/logging.h:32: #define LOGGING_INSIDE_LIBJINGLE On 2011/12/28 02:38:15, sergeyu wrote: > Can we define this in the gyp in the same way > SAFE_TO_DEFINE_TALK_BASE_LOGGING_MACROS was defined there? Yes, doing this is an improvement since we no longer need to worry about the include order (which is how the suggested solution detected whether or not the file is included by libjingle code or (transitively) by chromium code).
I have some more comments, majority of them are just style nits. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:5: #include "talk/base/logging.h" nit: better to include it via full path "third_party/..." http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:18: const char * FindLabel(int value, const ConstantLabel entries[]) { nit: "char*" without space http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:20: if (value == entries[i].value) { nit: no need to have braces here. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:27: std::string ErrorName(int err, const ConstantLabel * err_table) { nit: "ConstantLabel*" without space http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:45: LogEHelper::LogEHelper(const char* file, int line, LoggingSeverity severity, nit: each argument must be on a separate line if they don't fit on one line. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:47: : file_name_(file), line_(line), severity_(severity), extra_(extra) {} nit: it's more readable if each value is initialized on a separate line. nit: the closing brace } should be on a new line. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:65: #if WIN32 Chromium uses OS_WIN instead of WIN32 http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:85: #if OSX OS_MACOSX http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:102: void LogMultiline(LoggingSeverity level, const char* label, bool input, Is is just a copy copy of code from libjingle? If so, I think it's better to state it here. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:32: #include "config.h" // NOLINT nit: I guess we can remove this http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:54: const char * label; nit: "char*" without space http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:59: const char * FindLabel(int value, const ConstantLabel entries[]); nit: "char*" without space http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:60: std::string ErrorName(int err, const ConstantLabel* err_table); It looks strange that these two methods are here - they have nothing to do with logging. Maybe add TODO to move them http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, nit: In Chromium each argument must be on its own line if all of them don't fit on one line. I.e. this code must be formatted as follows: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, const char* module = NULL) http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, code style doesn't allow default argument values. Can we avoid them here? http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, Is it possible to change this code so that we don't expose this function in the header? E.g. roll it into LogEHelper(). Don't worry about it if it is too much work. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:164: VLOG_IS_ON(talk_base::sev) nit: this can fit on the previous line http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:166: VLOG_IS_ON(sev) nit: this can fit on the previous line http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:169: VLOG(sev) same here http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:172: LOG_V(talk_base::sev) same here http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:185: #define LOG_E_BASE(file_name, line_number, sev) \ As far as I can see, this is used in only one place in logging.cc. Do we need it? http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:188: true) nit: this can feet on the previous line http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:192: ? (void) 0 \ nit: operators should not be wrapped, ? must be on the previous line. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & move : to the previos line http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & It's better to wrap this expression in parentheses, even if it is not in libjingle version. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:200: talk_base::ERRCTX_ ## ctx, err , ##__VA_ARGS__)).stream() How does this work? GenerateExtra accepts no more than three arguments, so this code will compile only when __VA_ARGS__ contains only one argument. If __VA_ARGS__ always contains only one argument than why we need to use ... at all? http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:235: // TODO(?): Add an "assert" wrapper that logs in the same manner. nit: remove this TODO
Updated according to comments. Please have another go at reviewing. Thanks! http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:5: #include "talk/base/logging.h" On 2012/01/11 02:25:49, sergeyu wrote: > nit: better to include it via full path "third_party/..." Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:18: const char * FindLabel(int value, const ConstantLabel entries[]) { On 2012/01/11 02:25:49, sergeyu wrote: > nit: "char*" without space Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:20: if (value == entries[i].value) { On 2012/01/11 02:25:49, sergeyu wrote: > nit: no need to have braces here. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:27: std::string ErrorName(int err, const ConstantLabel * err_table) { On 2012/01/11 02:25:49, sergeyu wrote: > nit: "ConstantLabel*" without space Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:45: LogEHelper::LogEHelper(const char* file, int line, LoggingSeverity severity, On 2012/01/11 02:25:49, sergeyu wrote: > nit: each argument must be on a separate line if they don't fit on one line. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:47: : file_name_(file), line_(line), severity_(severity), extra_(extra) {} On 2012/01/11 02:25:49, sergeyu wrote: > nit: it's more readable if each value is initialized on a separate line. > nit: the closing brace } should be on a new line. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:65: #if WIN32 On 2012/01/11 02:25:49, sergeyu wrote: > Chromium uses OS_WIN instead of WIN32 Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:85: #if OSX On 2012/01/11 02:25:49, sergeyu wrote: > OS_MACOSX Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:102: void LogMultiline(LoggingSeverity level, const char* label, bool input, On 2012/01/11 02:25:49, sergeyu wrote: > Is is just a copy copy of code from libjingle? If so, I think it's better to > state it here. It is. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:32: #include "config.h" // NOLINT On 2012/01/11 02:25:49, sergeyu wrote: > nit: I guess we can remove this Correct, this is only used when building libjingle standalone. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:54: const char * label; On 2012/01/11 02:25:49, sergeyu wrote: > nit: "char*" without space Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:59: const char * FindLabel(int value, const ConstantLabel entries[]); On 2012/01/11 02:25:49, sergeyu wrote: > nit: "char*" without space Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:60: std::string ErrorName(int err, const ConstantLabel* err_table); On 2012/01/11 02:25:49, sergeyu wrote: > It looks strange that these two methods are here - they have nothing to do with > logging. Maybe add TODO to move them Well, they are used to provide (a shorthand for) text information when logging. The general comment above explains how ErrorName can be used. FindLabel is e.g. used in talk/base/schanneladapter.cc. This could potentially be moved to a separate file since the functionality is orthogonal to the logging. However, I don't think adding a comment is appropriate here as it may be confusing since the change should be made in libjingle and not in this override file. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, On 2012/01/11 02:25:49, sergeyu wrote: > code style doesn't allow default argument values. Can we avoid them here? There are some exceptions to the rule... I don't see a way that I can avoid default parameters here without making massive changes to libjingle (everywhere this macro is used but not all parameters passed (I verified that they exist by removing the default parameters and tried to compile)). http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, On 2012/01/11 02:25:49, sergeyu wrote: > nit: In Chromium each argument must be on its own line if all of them don't fit > on one line. I.e. this code must be formatted as follows: > std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, > int err = 0, > const char* module = NULL) Interesting, that's different from what the second example in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... . Good to know. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:137: std::string GenerateExtra(LogErrorContext err_ctx = ERRCTX_NONE, int err = 0, On 2012/01/11 02:25:49, sergeyu wrote: > Is it possible to change this code so that we don't expose this function in the > header? E.g. roll it into LogEHelper(). Don't worry about it if it is too much > work. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:164: VLOG_IS_ON(talk_base::sev) On 2012/01/11 02:25:49, sergeyu wrote: > nit: this can fit on the previous line Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:166: VLOG_IS_ON(sev) On 2012/01/11 02:25:49, sergeyu wrote: > nit: this can fit on the previous line Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:169: VLOG(sev) On 2012/01/11 02:25:49, sergeyu wrote: > same here Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:172: LOG_V(talk_base::sev) On 2012/01/11 02:25:49, sergeyu wrote: > same here Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:185: #define LOG_E_BASE(file_name, line_number, sev) \ On 2012/01/11 02:25:49, sergeyu wrote: > As far as I can see, this is used in only one place in logging.cc. Do we need > it? We don't need it since I can just expand the macro in the one place it is used. However, I think it makes the code easier to understand since it corresponds directly to VLOG. I have put it in logging.cc since it isn't needed anywhere else. Please let me know if this is OK. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:188: true) On 2012/01/11 02:25:49, sergeyu wrote: > nit: this can feet on the previous line Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:192: ? (void) 0 \ On 2012/01/11 02:25:49, sergeyu wrote: > nit: operators should not be wrapped, ? must be on the previous line. Totally agree. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & On 2012/01/11 02:25:49, sergeyu wrote: > move : to the previos line Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & On 2012/01/11 02:25:49, sergeyu wrote: > It's better to wrap this expression in parentheses, even if it is not in > libjingle version. I'm not sure I understand what you mean here can you please elaborate? http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:200: talk_base::ERRCTX_ ## ctx, err , ##__VA_ARGS__)).stream() On 2012/01/11 02:25:49, sergeyu wrote: > How does this work? GenerateExtra accepts no more than three arguments, so this > code will compile only when __VA_ARGS__ contains only one argument. If > __VA_ARGS__ always contains only one argument than why we need to use ... at > all? I believe it's because macros don't support default parameters. The only parameter that may or may not be passed when calling LOG_E is the very last one. I was able to remove the default parameters in LogEHelper for all but the last argument which seems to indicate that my assumption may be correct. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:235: // TODO(?): Add an "assert" wrapper that logs in the same manner. On 2012/01/11 02:25:49, sergeyu wrote: > nit: remove this TODO Done.
Looks mostly good now, but I have few more nits. Particularly see my suggestion about LOG_SEVERITY_PRECONDITION. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & On 2012/01/11 19:16:58, hellner wrote: > On 2012/01/11 02:25:49, sergeyu wrote: > > It's better to wrap this expression in parentheses, even if it is not in > > libjingle version. > > I'm not sure I understand what you mean here can you please elaborate? I was suggesting to write it as follows, notice () around ?: #define LOG_SEVERITY_PRECONDITION(sev) \ (!(LOG_CHECK_LEVEL_V(sev)) ? (void) 0 : talk_base::LogMessageVoidify()) & But now I see that's not the desired order of operations. I must say that it's too obscure. I believe that it would be better to make the right part of the & operator a parameter of this macro. Take a look at src/base/logging.h - it contains LAZY_STREAM() macro which is analogous to this one. Can we similarly add stream parameter here? Or we could just reuse LAZY_STREAM() macro here. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:200: talk_base::ERRCTX_ ## ctx, err , ##__VA_ARGS__)).stream() On 2012/01/11 19:16:58, hellner wrote: > On 2012/01/11 02:25:49, sergeyu wrote: > > How does this work? GenerateExtra accepts no more than three arguments, so > this > > code will compile only when __VA_ARGS__ contains only one argument. If > > __VA_ARGS__ always contains only one argument than why we need to use ... at > > all? > > I believe it's because macros don't support default parameters. > > The only parameter that may or may not be passed when calling LOG_E is the very > last one. I was able to remove the default parameters in LogEHelper for all but > the last argument which seems to indicate that my assumption may be correct. Ah, I see it now. The tricky part is that __VA_ARGS__ removes the preceding comma when written with ##. But we always specify first two parameters for GenerateExtra(). Do they need to have default values. http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... File jingle/glue/logging_unittest.cc (right): http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... jingle/glue/logging_unittest.cc:40: bool ContainsString(const std::string& original, const char* string_to_match) { nit: static http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... jingle/glue/logging_unittest.cc:44: bool Initialize(int verbosity_level) { nit: static http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:52: std::string GenerateExtra(LogErrorContext err_ctx, int err, nit: mark as static or move to nameless namespace nit: one line per argument http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:62: #if OS_WIN32 It is OS_WIN, not OS_WIN32. The canonical form is "#if defined(OS_WIN)" http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:82: #if OS_MACOSX same here
Updated according to comments. Please have another go at reviewing. Thanks! http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:193: : talk_base::LogMessageVoidify() & On 2012/01/11 19:54:48, sergeyu wrote: > On 2012/01/11 19:16:58, hellner wrote: > > On 2012/01/11 02:25:49, sergeyu wrote: > > > It's better to wrap this expression in parentheses, even if it is not in > > > libjingle version. > > > > I'm not sure I understand what you mean here can you please elaborate? > > I was suggesting to write it as follows, notice () around ?: > #define LOG_SEVERITY_PRECONDITION(sev) \ > (!(LOG_CHECK_LEVEL_V(sev)) ? (void) 0 : talk_base::LogMessageVoidify()) & > > But now I see that's not the desired order of operations. I must say that it's > too obscure. I believe that it would be better to make the right part of the & > operator a parameter of this macro. Take a look at src/base/logging.h - it > contains LAZY_STREAM() macro which is analogous to this one. Can we similarly > add stream parameter here? Or we could just reuse LAZY_STREAM() macro here. Good idea. Done. http://codereview.chromium.org/8991010/diff/14001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:200: talk_base::ERRCTX_ ## ctx, err , ##__VA_ARGS__)).stream() On 2012/01/11 19:54:48, sergeyu wrote: > On 2012/01/11 19:16:58, hellner wrote: > > On 2012/01/11 02:25:49, sergeyu wrote: > > > How does this work? GenerateExtra accepts no more than three arguments, so > > this > > > code will compile only when __VA_ARGS__ contains only one argument. If > > > __VA_ARGS__ always contains only one argument than why we need to use ... at > > > all? > > > > I believe it's because macros don't support default parameters. > > > > The only parameter that may or may not be passed when calling LOG_E is the > very > > last one. I was able to remove the default parameters in LogEHelper for all > but > > the last argument which seems to indicate that my assumption may be correct. > > Ah, I see it now. The tricky part is that __VA_ARGS__ removes the preceding > comma when written with ##. But we always specify first two parameters for > GenerateExtra(). Do they need to have default values. GenerateExtra no longer has any default parameters. The default parameters were moved to the constructor of LogEHelper when GenerateExtra was moved to the .cc file. Only the last argument needs to have a default parameter. http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... File jingle/glue/logging_unittest.cc (right): http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... jingle/glue/logging_unittest.cc:40: bool ContainsString(const std::string& original, const char* string_to_match) { On 2012/01/11 19:54:48, sergeyu wrote: > nit: static Done. http://codereview.chromium.org/8991010/diff/21001/jingle/glue/logging_unittes... jingle/glue/logging_unittest.cc:44: bool Initialize(int verbosity_level) { On 2012/01/11 19:54:48, sergeyu wrote: > nit: static Done. http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/01/11 19:54:48, sergeyu wrote: > 2012 Done. http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:52: std::string GenerateExtra(LogErrorContext err_ctx, int err, On 2012/01/11 19:54:48, sergeyu wrote: > nit: mark as static or move to nameless namespace > nit: one line per argument Done. http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:62: #if OS_WIN32 On 2012/01/11 19:54:48, sergeyu wrote: > It is OS_WIN, not OS_WIN32. The canonical form is "#if defined(OS_WIN)" Done. http://codereview.chromium.org/8991010/diff/21001/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:82: #if OS_MACOSX On 2012/01/11 19:54:48, sergeyu wrote: > same here Done.
Couple more minor nits. LGTM otherwise. Thanks for implementing proper solution for libjingle logging! http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:153: #ifdef LOGGING_INSIDE_LIBJINGLE #if defined(LOGGING_INSIDE_LIBJINGLE) http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:180: #ifdef WIN32 #if defined(OS_WIN) http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:187: #elif POSIX #else
Updated according to comments. Also solved build errors on Mac and Windows, please have another go at reviewing since there are some non-trivial changes. Especially look at logging.h, line 172 (patch set 11). Thanks! http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:153: #ifdef LOGGING_INSIDE_LIBJINGLE On 2012/01/12 00:11:36, sergeyu wrote: > #if defined(LOGGING_INSIDE_LIBJINGLE) Done. http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:180: #ifdef WIN32 On 2012/01/12 00:11:36, sergeyu wrote: > #if defined(OS_WIN) Done. http://codereview.chromium.org/8991010/diff/24002/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:187: #elif POSIX On 2012/01/12 00:11:36, sergeyu wrote: > #else Done.
two more nits. LGTM otherwise. http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:18: #define snprintf _snprintf Alternatively you can include base/string_util.h and use base::snprintf() http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:171: // unless __VA_ARGS__ is at the end. TODO(hellner): find reference for this. nit: TODOs are usually placed in the beginning of comment line.
Fixed the nits. Also had to update the jingle DEPS so that it may depend on libjingle overrides as well as regular libjingle. Please see the change to jingle/DEPS for that. I butterfingered a change to jingle/jingle.gyp that was reverted but it is the same as in the first patch set. Please have another look. Thanks! http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.cc (right): http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.cc:18: #define snprintf _snprintf On 2012/01/13 01:34:41, sergeyu wrote: > Alternatively you can include base/string_util.h and use base::snprintf() Good idea. Done. http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... File third_party/libjingle/overrides/talk/base/logging.h (right): http://codereview.chromium.org/8991010/diff/29005/third_party/libjingle/overr... third_party/libjingle/overrides/talk/base/logging.h:171: // unless __VA_ARGS__ is at the end. TODO(hellner): find reference for this. On 2012/01/13 01:34:41, sergeyu wrote: > nit: TODOs are usually placed in the beginning of comment line. Done.
one nit in the DEPS file. http://codereview.chromium.org/8991010/diff/37002/jingle/DEPS File jingle/DEPS (right): http://codereview.chromium.org/8991010/diff/37002/jingle/DEPS#newcode3 jingle/DEPS:3: "+third_party/libjingle/overrides" It's better to move this to a separate DEPS file in jingle/glue and add some comments to describe that the test needs it.
Updated according to comment. I had a look at the try-bots and it seems like this cl is not causing any new problems. Would you mind having a final look before I send it to the commit bots. Thanks for the quick turnaround time! http://codereview.chromium.org/8991010/diff/37002/jingle/DEPS File jingle/DEPS (right): http://codereview.chromium.org/8991010/diff/37002/jingle/DEPS#newcode3 jingle/DEPS:3: "+third_party/libjingle/overrides" On 2012/01/13 19:45:04, sergeyu wrote: > It's better to move this to a separate DEPS file in jingle/glue and add some > comments to describe that the test needs it. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hellner@google.com/8991010/30008
Change committed as 117917 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
