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

Issue 42391: Add some UBC eviction code after talking to Amit (and it doesn't even require... (Closed)

Created:
11 years, 9 months ago by TVL
Modified:
9 years, 7 months ago
Reviewers:
Mark Mentovai
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Add some UBC eviction code after talking to Amit (and it doesn't even require a kext) :) Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12090

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -3 lines) Patch
M base/test_file_util_mac.cc View 1 chunk +25 lines, -3 lines 4 comments Download

Messages

Total messages: 2 (0 generated)
TVL
11 years, 9 months ago (2009-03-19 14:41:42 UTC) #1
Mark Mentovai
11 years, 9 months ago (2009-03-19 14:50:07 UTC) #2
L(ooks) good to me on the code.

L(ess) good on the comments.

http://codereview.chromium.org/42391/diff/1/2
File base/test_file_util_mac.cc (right):

http://codereview.chromium.org/42391/diff/1/2#newcode7
Line 7: #include <sys/mman.h>
Come on, what alphabet are we using today?

http://codereview.chromium.org/42391/diff/1/2#newcode16
Line 16: // talking w/ Amit Singh, the safest is to mmap the file, do a msync to
I would do "an" msync unless you have a novel way to pronounce msync that
doesn't start with "em."

Also, this could be revised to be a little clearer, like by putting "then"
before "do."

http://codereview.chromium.org/42391/diff/1/2#newcode20
Line 20: // We use MemoryMappedFile because it mmaps MAP_FILE | MAP_SHARED (it
doesn't
That's kind of obvious from reading MemoryMappedFile::MapFileToMemory.  This
comment isn't contributing anything really except implying that MAP_FILE |
MAP_SHARED is needed to make this scheme work.  Is it?  If so, it ought to be
explicit.

http://codereview.chromium.org/42391/diff/1/2#newcode24
Line 24: file_util::MemoryMappedFile mmap;
I would not name a variable mmap, mmap is the name of a system call and this is
confusing.

Powered by Google App Engine
This is Rietveld 408576698