Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832309:
Make printf and LOG output to cmd console for Windows (Closed)
|
Created:
8 years, 4 months ago by jbates Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake printf and LOG output to cmd console for Windows
BUG=142722
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152432
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=152566
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152722
Patch Set 1 #Patch Set 2 : minor fix #Patch Set 3 : compile #
Total comments: 4
Patch Set 4 : brettw, maruel feedback #
Total comments: 5
Patch Set 5 : added error checking #Patch Set 6 : merge / no change #Patch Set 7 : wez feedback + removed stdin support #
Total comments: 4
Patch Set 8 : wez feedback #
Messages
Total messages: 22 (0 generated)
ptal
lgtm https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... base/process_util_win.cc:73: void AttachToConsole() { Maybe just rename this one and move out of the anon namespace so there's only on version (rather than having both AttachToConsole and RouteStdioToConsole as doing the same thing)?
https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... base/process_util_win.cc:79: if (!AttachConsole(ATTACH_PARENT_PROCESS)) { This already takes care of not doing it 2 times in a row. Lines 74-77 are not needed.
https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... base/process_util_win.cc:73: void AttachToConsole() { On 2012/08/19 23:26:46, brettw wrote: > Maybe just rename this one and move out of the anon namespace so there's only on > version (rather than having both AttachToConsole and RouteStdioToConsole as > doing the same thing)? Done. https://chromiumcodereview.appspot.com/10832309/diff/5001/base/process_util_w... base/process_util_win.cc:79: if (!AttachConsole(ATTACH_PARENT_PROCESS)) { On 2012/08/20 12:42:29, Marc-Antoine Ruel wrote: > This already takes care of not doing it 2 times in a row. Lines 74-77 are not > needed. Done.
I preferred the name AttachToConsole but I don't mind. lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbates@chromium.org/10832309/11001
Change committed as 152432
https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... base/process_util_win.cc:144: if (result == ERROR_INVALID_HANDLE || result == ERROR_INVALID_HANDLE) { Why are we testing result twice against the same value? https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... base/process_util_win.cc:153: *stdout = *_fdopen(raw_out, "w"); Can't you use freopen("CONOUT$", "w", stdout) here?
https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... base/process_util_win.cc:144: if (result == ERROR_INVALID_HANDLE || result == ERROR_INVALID_HANDLE) { On 2012/08/21 18:08:41, Wez wrote: > Why are we testing result twice against the same value? How can you be sure if you don't check twice? lolz. This was a code move. I'll fix it though. https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... base/process_util_win.cc:153: *stdout = *_fdopen(raw_out, "w"); On 2012/08/21 18:08:41, Wez wrote: > Can't you use freopen("CONOUT$", "w", stdout) here? Again, code move, but this code is based on the above microsoft support thread. I'd rather not diverge much from the MS example unless it's important.
The original patch crashed on some dude's Canary run on XP with --enable-logging... I'm guessing one of the unchecked errors cascaded into a crash. The latest CL adds error checking that should catch any runtime problems. PTAL
https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/11001/base/process_util_... base/process_util_win.cc:153: *stdout = *_fdopen(raw_out, "w"); On 2012/08/21 18:58:01, jbates wrote: > On 2012/08/21 18:08:41, Wez wrote: > > Can't you use freopen("CONOUT$", "w", stdout) here? > > Again, code move, but this code is based on the above microsoft support thread. > I'd rather not diverge much from the MS example unless it's important. I just prefer the freopen() version because it is simpler and avoids the potential for issues on 64-bit builds that the current code has.
wez: ptal - thanks for the tip, this looks a lot cleaner.
https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... base/process_util_win.cc:155: if (freopen("CONOUT$", "w", stderr)) CONOUT$->CONERR$ https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... base/process_util_win.cc:157: You're not freopen()ing CONIN$ - is that intentional?
I've just realised that these changes are happening on the same CL for the original changes, that have already landed; I think it's preferred that a new CL be created for post-commit tweaks.
On 2012/08/21 23:38:50, Wez wrote: > I've just realised that these changes are happening on the same CL for the > original changes, that have already landed; I think it's preferred that a new CL > be created for post-commit tweaks. It was reverted, so it seems natural to continue the discussion here to maintain the diff history. I included the revert rev above. Is there a problem with this approach that makes it worth reviewers re-reviewing without history?
https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... File base/process_util_win.cc (right): https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... base/process_util_win.cc:155: if (freopen("CONOUT$", "w", stderr)) On 2012/08/21 23:37:02, Wez wrote: > CONOUT$->CONERR$ Done. https://chromiumcodereview.appspot.com/10832309/diff/11007/base/process_util_... base/process_util_win.cc:157: On 2012/08/21 23:37:02, Wez wrote: > You're not freopen()ing CONIN$ - is that intentional? Yes, there are no use cases for stdin that I can see, so doesn't seem worth the extra code.
On 2012/08/21 23:44:30, jbates wrote: > On 2012/08/21 23:38:50, Wez wrote: > > I've just realised that these changes are happening on the same CL for the > > original changes, that have already landed; I think it's preferred that a new > CL > > be created for post-commit tweaks. > > It was reverted, so it seems natural to continue the discussion here to maintain > the diff history. I included the revert rev above. Is there a problem with this > approach that makes it worth reviewers re-reviewing without history? Marc-Antoine IIRC argued that there should be a 1:1 correspondence between CLs and commits to avoid confusion; the new CL for a re-landed patch can simply link back to the original to capture any discussion.
On 2012/08/21 23:51:52, Wez wrote: > On 2012/08/21 23:44:30, jbates wrote: > > On 2012/08/21 23:38:50, Wez wrote: > > > I've just realised that these changes are happening on the same CL for the > > > original changes, that have already landed; I think it's preferred that a > new > > CL > > > be created for post-commit tweaks. > > > > It was reverted, so it seems natural to continue the discussion here to > maintain > > the diff history. I included the revert rev above. Is there a problem with > this > > approach that makes it worth reviewers re-reviewing without history? > > Marc-Antoine IIRC argued that there should be a 1:1 correspondence between CLs > and commits to avoid confusion; the new CL for a re-landed patch can simply link > back to the original to capture any discussion. IIRC the conclusion of that thread was "best judgement should apply". I think I've addressed most of MA's issues by keeping a clean history of commits and reverts on this CL, but let me know if you'd like me to split out in any case.
On 2012/08/22 00:01:03, jbates wrote: > On 2012/08/21 23:51:52, Wez wrote: > > On 2012/08/21 23:44:30, jbates wrote: > > > On 2012/08/21 23:38:50, Wez wrote: > > > > I've just realised that these changes are happening on the same CL for the > > > > original changes, that have already landed; I think it's preferred that a > > new > > > CL > > > > be created for post-commit tweaks. > > > > > > It was reverted, so it seems natural to continue the discussion here to > > maintain > > > the diff history. I included the revert rev above. Is there a problem with > > this > > > approach that makes it worth reviewers re-reviewing without history? > > > > Marc-Antoine IIRC argued that there should be a 1:1 correspondence between CLs > > and commits to avoid confusion; the new CL for a re-landed patch can simply > link > > back to the original to capture any discussion. > > IIRC the conclusion of that thread was "best judgement should apply". I think > I've addressed most of MA's issues by keeping a clean history of commits and > reverts on this CL, but let me know if you'd like me to split out in any case. That's not quite how I interpreted that thread, but LGTM for this CL as-is.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbates@chromium.org/10832309/20001
Change committed as 152722 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
