Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(386)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 18401005: PathUtilsTest valgrind patch (Closed)

Created:
7 years, 5 months ago by robertphillips
Modified:
7 years, 5 months ago
Reviewers:
dierk
Visibility:
Public.

Description

PathUtilsTest valgrind patch

Patch Set 1 #

Total comments: 1

Patch Set 2 : Valgrind patch with valid test. #

Patch Set 3 : Moved iostream include with other debugging code. #

Total comments: 22

Patch Set 4 : Fixed some nit's #

Patch Set 5 : got the rest of the nit's #

Patch Set 6 : and some more nit's... #

Patch Set 7 : done w nit's. #

Total comments: 2

Patch Set 8 : Fixed the space in method name. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -37 lines) Patch
M tests/PathUtilsTest.cpp View 1 2 3 4 5 6 7 4 chunks +58 lines, -37 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
robertphillips
https://codereview.chromium.org/18401005/diff/1/tests/PathUtilsTest.cpp File tests/PathUtilsTest.cpp (right): https://codereview.chromium.org/18401005/diff/1/tests/PathUtilsTest.cpp#newcode66 tests/PathUtilsTest.cpp:66: for (int x = 0; x < w; ++x) ...
7 years, 5 months ago (2013-07-13 17:01:08 UTC) #1
dierk
Fixed the test and it seems to run without any valgrind errors now too. Thanks ...
7 years, 5 months ago (2013-07-15 17:02:16 UTC) #2
dierk
lgtm
7 years, 5 months ago (2013-07-15 17:02:30 UTC) #3
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
7 years, 5 months ago (2013-07-15 17:14:17 UTC) #4
robertphillips
lgtm https://codereview.chromium.org/18401005/diff/5001/tests/PathUtilsTest.cpp File tests/PathUtilsTest.cpp (right): https://codereview.chromium.org/18401005/diff/5001/tests/PathUtilsTest.cpp#newcode16 tests/PathUtilsTest.cpp:16: prefix all with SK_ https://codereview.chromium.org/18401005/diff/5001/tests/PathUtilsTest.cpp#newcode17 tests/PathUtilsTest.cpp:17: #define NUM_IT ...
7 years, 5 months ago (2013-07-15 17:14:38 UTC) #5
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...
7 years, 5 months ago (2013-07-15 17:14:47 UTC) #6
robertphillips
committed as r10085
7 years, 5 months ago (2013-07-15 17:16:58 UTC) #7
dierk
https://codereview.chromium.org/18401005/diff/5001/tests/PathUtilsTest.cpp File tests/PathUtilsTest.cpp (right): https://codereview.chromium.org/18401005/diff/5001/tests/PathUtilsTest.cpp#newcode16 tests/PathUtilsTest.cpp:16: On 2013/07/15 17:14:39, robertphillips wrote: > prefix all with ...
7 years, 5 months ago (2013-07-15 17:28:21 UTC) #8
robertphillips
https://codereview.chromium.org/18401005/diff/15002/tests/PathUtilsTest.cpp File tests/PathUtilsTest.cpp (right): https://codereview.chromium.org/18401005/diff/15002/tests/PathUtilsTest.cpp#newcode47 tests/PathUtilsTest.cpp:47: does this compile?
7 years, 5 months ago (2013-07-16 15:11:50 UTC) #9
dierk
7 years, 5 months ago (2013-07-16 15:25:53 UTC) #10
Message was sent while issue was closed. 
https://codereview.chromium.org/18401005/diff/15002/tests/PathUtilsTest.cpp
File tests/PathUtilsTest.cpp (right):

https://codereview.chromium.org/18401005/diff/15002/tests/PathUtilsTest.cpp#n...
tests/PathUtilsTest.cpp:47: 
Yes, only because it is in a block comment. I'll fix it.

On 2013/07/16 15:11:50, robertphillips wrote:
> does this compile?

Powered by Google App Engine
This is Rietveld 408576698