Issue 6450095: Add -logFile option to Bench (Closed)
|
Created:
12 years, 3 months ago by EricB Modified:
12 years, 3 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd -logFile option to Bench
Logs to a file as well as stdout. Useful for Android,
where logging takes a different format.
Committed: https://code.google.com/p/skia/source/detail?r=4963
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 5
Patch Set 5 : #MessagesTotal messages: 9
https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp File bench/benchmain.cpp (right): https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp#newcode40 bench/benchmain.cpp:40: static void log_progress(const char msg[]) { printf("%s", msg); } Should I be concerned that calls to log_progress(const char msg[]) will not be copied to the file? If no code actually calls this directly, maybe it could be renamed to something else like _log_progress() or internal_log_progress() ? https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp#newcode388 bench/benchmain.cpp:388: SkDebugf(" -logFile : destination for writing log output.\n"); "in addition to stdout"
Sign in to reply to this message.
On 2012/08/06 15:51:29, epoger wrote: > https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp > File bench/benchmain.cpp (right): > > https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp#newcode40 > bench/benchmain.cpp:40: static void log_progress(const char msg[]) { > printf("%s", msg); } > Should I be concerned that calls to log_progress(const char msg[]) will not be > copied to the file? > > If no code actually calls this directly, maybe it could be renamed to something > else like _log_progress() or internal_log_progress() ? > > https://codereview.appspot.com/6450095/diff/1001/bench/benchmain.cpp#newcode388 > bench/benchmain.cpp:388: SkDebugf(" -logFile : destination for writing log > output.\n"); > "in addition to stdout" Patch set 4 places logging inside a class. Would it be a bad thing to do this kind of thing for SkDebugf globally?
Sign in to reply to this message.
LGTM at patchset 4, subject to the following quibbles. Maybe a future CL could move this into its own file for use by other modules? https://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp File bench/benchmain.cpp (right): https://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43 bench/benchmain.cpp:43: bool setLogFile(const char file[]) { Looking at https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... , I guess these methods should be named as follows: SetLogFile() LogError() LogProgress() https://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode54 bench/benchmain.cpp:54: void log_error(const SkString& str) { log_error(str.c_str()); } Please group the overloaded versions of each method name together. (Both versions of LogError, and then both versions of LogProgress) https://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode69 bench/benchmain.cpp:69: void fileWrite(const char msg[], size_t size) { maybe name this file_write() for consistency with the other private methods https://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode908 bench/benchmain.cpp:908: logger.log_progress(SkString("\n")); should be fine without the SkString wrapper, right?
Sign in to reply to this message.
http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp File bench/benchmain.cpp (right): http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43 bench/benchmain.cpp:43: bool setLogFile(const char file[]) { I'm not sure I agree. The leading cap is only for static functions. On 2012/08/06 17:18:55, epoger wrote: > Looking at > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... > , I guess these methods should be named as follows: > > SetLogFile() > LogError() > LogProgress()
Sign in to reply to this message.
On 2012/08/06 17:24:21, DerekS wrote: > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp > File bench/benchmain.cpp (right): > > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43 > bench/benchmain.cpp:43: bool setLogFile(const char file[]) { > I'm not sure I agree. The leading cap is only for static functions. "Externed functions or static class functions are camel-capped with an initial cap" I'm not sure if "externed" includes "public", but that's what it shows in the example... > > On 2012/08/06 17:18:55, epoger wrote: > > Looking at > > > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... > > , I guess these methods should be named as follows: > > > > SetLogFile() > > LogError() > > LogProgress()
Sign in to reply to this message.
The extern funcion mentioned in the doc is bool SkIsOdd(int n); which is separate from the static class function. On Mon, Aug 6, 2012 at 1:26 PM, <epoger@google.com> wrote: > On 2012/08/06 17:24:21, DerekS wrote: > >> http://codereview.appspot.com/**6450095/diff/6003/bench/**benchmain.cpp<http:... >> File bench/benchmain.cpp (right): >> > > > http://codereview.appspot.com/**6450095/diff/6003/bench/** > benchmain.cpp#newcode43<http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43> > >> bench/benchmain.cpp:43: bool setLogFile(const char file[]) { >> I'm not sure I agree. The leading cap is only for static functions. >> > > "Externed functions or static class functions are camel-capped with an > initial cap" > > I'm not sure if "externed" includes "public", but that's what it shows > in the example... > > > > On 2012/08/06 17:18:55, epoger wrote: >> > Looking at >> > >> > > https://sites.google.com/site/**skiadocs/developer-** > documentation/contributing-**code/coding-style-guidelines<https://sites.google.com/site/skiadocs/developer-documentation/contributing-code/coding-style-guidelines> > >> > , I guess these methods should be named as follows: >> > >> > SetLogFile() >> > LogError() >> > LogProgress() >> > > > https://codereview.appspot.**com/6450095/<https://codereview.appspot.com/6450... >
Sign in to reply to this message.
On 2012/08/06 17:26:23, epoger wrote: > On 2012/08/06 17:24:21, DerekS wrote: > > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp > > File bench/benchmain.cpp (right): > > > > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43 > > bench/benchmain.cpp:43: bool setLogFile(const char file[]) { > > I'm not sure I agree. The leading cap is only for static functions. > > "Externed functions or static class functions are camel-capped with an initial > cap" > > I'm not sure if "externed" includes "public", but that's what it shows in the > example... > > > > > On 2012/08/06 17:18:55, epoger wrote: > > > Looking at > > > > > > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... > > > , I guess these methods should be named as follows: > > > > > > SetLogFile() > > > LogError() > > > LogProgress() While that doc does not state a rule for class member functions (aside from static functions), a brief scan of some core Skia classes shows camel-case with leading lower-case.
Sign in to reply to this message.
On 2012/08/06 17:58:53, EricB wrote: > On 2012/08/06 17:26:23, epoger wrote: > > On 2012/08/06 17:24:21, DerekS wrote: > > > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp > > > File bench/benchmain.cpp (right): > > > > > > > http://codereview.appspot.com/6450095/diff/6003/bench/benchmain.cpp#newcode43 > > > bench/benchmain.cpp:43: bool setLogFile(const char file[]) { > > > I'm not sure I agree. The leading cap is only for static functions. > > > > "Externed functions or static class functions are camel-capped with an initial > > cap" > > > > I'm not sure if "externed" includes "public", but that's what it shows in the > > example... > > > > > > > > On 2012/08/06 17:18:55, epoger wrote: > > > > Looking at > > > > > > > > > > https://sites.google.com/site/skiadocs/developer-documentation/contributing-c... > > > > , I guess these methods should be named as follows: > > > > > > > > SetLogFile() > > > > LogError() > > > > LogProgress() > > While that doc does not state a rule for class member functions (aside from > static functions), a brief scan of some core Skia classes shows camel-case with > leading lower-case. Gotcha. Please ignore my naming comments... seems like we should update the style guide to be more complete.
Sign in to reply to this message.
|
