Please don't review in detail yet, since I haven't tried it on win/mac due to
trybot hosedness.  But check out compiler_specific and string_util changes.

I didn't look at the whole thing (because you said not to) but I like the
strategy.

drive by

http://codereview.chromium.org/339059/diff/1/2
File base/compiler_specific.h (right):

http://codereview.chromium.org/339059/diff/1/2#newcode76
Line 76: // |dots_param| is the one-base index of the "..." parameter.
you say "one-based" but it looks like it has to support zero meaning missing
(ie-va_list).  would be it better to have another macro for va_list form incase
some compiler doesn't do this via the zero trick?

On 2009/10/29 01:57:05, TVL wrote:
> http://codereview.chromium.org/339059/diff/1/2#newcode76
> Line 76: // |dots_param| is the one-base index of the "..." parameter.
> you say "one-based" but it looks like it has to support zero meaning missing
> (ie-va_list).  would be it better to have another macro for va_list form
incase
> some compiler doesn't do this via the zero trick?

I'd rather wait until we see a different compiler before we plan too much ahead;
it's only used in about six places so it's not hard to tweak.  I will update the
doc to mention the 0

this doesn't work because %z doesn't work on windows.  I need to use %I, which
means I'll need a PRId64-like macro.  :(

I think I figured out the Windows problem, so this is ready for review.

http://codereview.chromium.org/339059/diff/8001/9001
File base/compiler_specific.h (right):

http://codereview.chromium.org/339059/diff/8001/9001#newcode74
Line 74: // Annotate a function that it's using a printf-style format string.
"Annotate a function that it's"?

Did you mean: "Annotate a function that's"?

Huge Selection and Amazing Prices. Free Shipping on Orders Over $25!

http://codereview.chromium.org/339059/diff/8001/9001#newcode77
Line 77: #define PRINTF_FORMAT(format_param,dots_param) \
Nit: spaces after commas.  Throughout.

http://codereview.chromium.org/339059/diff/8001/9001#newcode78
Line 78: __attribute__((format(printf,format_param,dots_param)))
4-space indent on continuation lines.

http://codereview.chromium.org/339059/diff/8001/9001#newcode80
Line 80: // This appears to be as-of-yet unimplemented on my old ghardy box.
"This doesn't appear to be implemented on my old ghardy box."

But is the distribution even relevant?

And what the f is ghardy?  :)

http://codereview.chromium.org/339059/diff/8001/9001#newcode82
Line 82: // version dependent.
You might want to reference http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38308,
which is the closest thing that I found in GCC Bugzilla.  That, in turn,
references an old patch.

http://codereview.chromium.org/339059/diff/8001/9002
File base/format_macros.h (right):

http://codereview.chromium.org/339059/diff/8001/9002#newcode21
Line 21: //   printf("xyz:%"PRIsz, x);
Nit: the example above using PRId64 puts a space between the " and the macro. 
Be consistent.

http://codereview.chromium.org/339059/diff/8001/9002#newcode45
Line 45: #define PRIsz "%zu"
Hmm.  A certain other codebase uses PRI?S for these, like PRIdS, PRIxS, and the
environmentally-friendly and most commonly used PRIuS.

I think we should use these names too.  It's consistent with that certain
codebase.  It's also consistent with the standard <inttypes.h> macros in that it
puts the modifier first and the type second.

PRIuS is also consistent with our style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

...although the style guide's code snippet doesn't use %z when __LP64 isn't
defined, and we want to use %z for non-Windows regardless.  I'll fix the style
guide.  Except I can't now because I'm on an airplane and my workstation in my
office is unplugged.

Finally, you shouldn't put the % inside this macro.  None of the other PRI*
macros do, and your example comment on line 21 indicates that this macro doesn't
include its own % either.  The "%" here is bad for consistency and bad for users
that might want to put additional specifiers on this, like field width.

http://codereview.chromium.org/339059/diff/8001/9002#newcode63
Line 63: #define WidePRIx64 L"I64x"
Do we want to define WidePRIuS too?

http://codereview.chromium.org/339059/diff/8001/9006
File base/string_util.h (right):

http://codereview.chromium.org/339059/diff/8001/9006#newcode47
Line 47: PRINTF_FORMAT(3, 0);
compiler_specific.h said that the macro arguments were 1-based, but now you're
using 0?

I get it.  v*printf doesn't have dots.  But I don't see the 0 documented at
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code...,
and you didn't mention it in compiler_specific.h.

http://codereview.chromium.org/339059/diff/8001/9007
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9007#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Can we get a test of StringAppendV in here to make sure that we have a call site
somewhere in our codebase, to validate the maybe-undocumented behavior of using
0 as the dots parameter?

http://codereview.chromium.org/339059/diff/8001/9008
File base/trace_event.cc (right):

http://codereview.chromium.org/339059/diff/8001/9008#newcode142
Line 142: reinterpret_cast<unsigned long>(id),
id is a void*, so this should be %p (and no cast), right?

http://codereview.chromium.org/339059/diff/8001/9014
File chrome/browser/net/chrome_url_request_context_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9014#newcode167
Line 167: SCOPED_TRACE(StringPrintf("Test["PRIsz"] %s", i,
Spaces around the macro, maybe?

http://codereview.chromium.org/339059/diff/8001/9015
File chrome/browser/net/dns_host_info.cc (left):

http://codereview.chromium.org/339059/diff/8001/9015#oldcode5
Line 5: // See header file for description of class
Yeah, I mean, duh, right?

http://codereview.chromium.org/339059/diff/8001/9016
File chrome/browser/safe_browsing/protocol_parser.cc (right):

http://codereview.chromium.org/339059/diff/8001/9016#newcode119
Line 119: request->append(StringPrintf(PRIsz":"PRIsz"\n",
Yeah, really, spaces around the quotes.  I didn't mention all of the ones I
found and I'm not gonna say it again, but this one is particularly unreadable
without the spaces.  Here's why:

...codefragment...z":"PRIsz":"PRIsz":"PRIsz":"P...codefragment...

Which ones are the string literals?  "PRIsz" or ":"?  The sapces would help
clear up this confusion.

http://codereview.chromium.org/339059/diff/8001/9019
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9019#newcode466
Line 466: FilePath path(StringPrintf("/extension%zu", i));
Use the Toyota here too, even though it's guaranteed not to be seen by Windows.

For that matter, for maximum correctness, use it on line 464, even where it will
be seen by Windows.

http://codereview.chromium.org/339059/diff/8001/9019#newcode488
Line 488: FilePath path(StringPrintf("/extension%zu", i));
As above.

http://codereview.chromium.org/339059/diff/8001/9021
File media/base/video_frame_impl_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9021#newcode87
Line 87: SCOPED_TRACE(StringPrintf("Checking (%zu, %zu)", row, col));
Use the macros.

http://codereview.chromium.org/339059/diff/8001/9022
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/339059/diff/8001/9022#newcode154
Line 154: return StringPrintf("%s://0x%p", kProtocol,
static_cast<void*>(protocol));
%p will give you the 0x (unless the value is 0).  This should just be "%s://%p",
innit?

http://codereview.chromium.org/339059/diff/8001/9023
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/339059/diff/8001/9023#newcode1175
Line 1175: return StringPrintf("name: %s value: %s path: %s creation: %"PRIu64,
PRIu64 (emphasis "u") but...

http://codereview.chromium.org/339059/diff/8001/9023#newcode1177
Line 1177: static_cast<int64>(creation_date_.ToTimeT()));
int64 (emphasis "no u")?

Fight!

I would choose int64/PRId64 because time_t is  (signed) long on the systems we
care about, and supports values prior to the epoch.

http://codereview.chromium.org/339059/diff/8001/9026
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9026#newcode13
Line 13: #include "base/basictypes.h"
Comes from the header now, no longer needed.

http://codereview.chromium.org/339059/diff/8001/9026#newcode20
Line 20: #include "net/base/escape.h"
Same.

http://codereview.chromium.org/339059/diff/8001/9029
File net/ftp/ftp_directory_listing_parsers_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9029#newcode73
Line 73: 
What's the new blank line for?

If anything, I think it'd be more readable if the blank line came before the
loop "for" line.  But this file doesn't do that elsewhere.

http://codereview.chromium.org/339059/diff/8001/9030
File net/http/http_cache.cc (right):

http://codereview.chromium.org/339059/diff/8001/9030#newcode215
Line 215: url.insert(0, StringPrintf("%"PRId64"/",
request->upload_data->identifier()));
80, split this line up.

http://codereview.chromium.org/339059/diff/8001/9036
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/339059/diff/8001/9036#newcode708
Line 708: r.SetExtraRequestHeaders(StringPrintf("Range: bytes=%zu-%zu\n",
Use the macro!

http://codereview.chromium.org/339059/diff/8001/9037
File net/url_request/url_request_view_net_internals_job.cc (right):

http://codereview.chromium.org/339059/diff/8001/9037#newcode225
Line 225: host_cache->cache_duration_ms()));
Wow, I'm surprised that cache_duration_ms is size_t, but it is.

http://codereview.chromium.org/339059/diff/8001/9038
File webkit/glue/dom_serializer.cc (right):

http://codereview.chromium.org/339059/diff/8001/9038#newcode90
Line 90: "\n<!-- saved from url=(%04zd)%s -->\n";
Use the macro, this should wind up as |"%04" PRIdS|.

Thanks for the careful review.  I unfortunately had to rebase to continue
running this on the try servers, so the diff is large, but I think I've
addressed your comments.

ping

http://codereview.chromium.org/339059/diff/12001/13001
File base/compiler_specific.h (right):

http://codereview.chromium.org/339059/diff/12001/13001#newcode74
Line 74: // Annotate a function that it's using a printf-style format string.
I was going to start reviewing this (late, I know, sorry, but I've been working
down the M4 bugs) but I'm disappointed to find that you didn't address the first
comment I made on the last pass.

Before I look at this, could you run through my comments again to make sure
you've hit everything?

Specifically, "annotate something that it's using something else" seems kind of
like gibberish.

http://codereview.chromium.org/339059/diff/12001/13001#newcode92
Line 92: #define PRINTF_FORMAT(x,y)
Also, put a space between x and y here and on the next line.

On 2009/11/18 07:52:20, Mark Mentovai wrote:
> I was going to start reviewing this (late, I know, sorry, but I've been
working
> down the M4 bugs) but I'm disappointed to find that you didn't address the
first
> comment I made on the last pass.
> 
> Before I look at this, could you run through my comments again to make sure
> you've hit everything?

Really sorry, I missed the first one.
I *believe* I got 'em all, but you're much more careful than I am.

Your message included:
"Huge Selection and Amazing Prices. Free Shipping on Orders Over $25!"
which I think I glazed over as an ad.

On 2009/11/18 20:55:27, Mark Mentovai wrote:
> LGTM

Jeez, you'd think I could grep for \SPRI to find the places where I forgot
spaces or something.  :(

Thanks again for lookign.

> 
> http://codereview.chromium.org/339059/diff/15001/15008
> File base/string_util_unittest.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15008#newcode919
> Line 919: static void StringAppendVTestHelper(std::string* out,
> Maybe mark this as PRINTF_FORMAT too.
> 
> http://codereview.chromium.org/339059/diff/15001/15012
> File chrome/app/chrome_dll_main.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15012#newcode463
> Line 463: std::string statsfile = StringPrintf("%s-%ul",
chrome::kStatsFilename,
> The cast is to unsigned int but you've got %ul.
> 
> Either cast to unsigned long or use %u.
> 
> http://codereview.chromium.org/339059/diff/15001/15018
> File chrome/browser/safe_browsing/safe_browsing_database_unittest.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15018#newcode1098
> Line 1098: LOG(INFO) << StringPrintf("I/O Read Bytes: %"PRIu64,
> "I/O Read Bytes: %" PRIu64,
> 
> (with the space)
> 
> http://codereview.chromium.org/339059/diff/15001/15020
> File chrome/common/extensions/extension_unittest.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15020#newcode467
> Line 467: // might make use of a WidePRIuS; let's not encourage any more.)
> OK, that's fair enough.  But wouldn't it be L"%Iu" for a size_t on Windows
> instead of L"%d"?
> 
> http://codereview.chromium.org/339059/diff/15001/15020#newcode492
> Line 492: // let's just work around it here.
> Same.
> 
> Oh, I guess there are two locations...
> 
> http://codereview.chromium.org/339059/diff/15001/15028
> File net/disk_cache/sparse_control.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15028#newcode44
> Line 44: return StringPrintf("Range_%s:%"PRIx64":%"PRIx64, base_name.c_str(),
> Spaces around the macro.
> 
> http://codereview.chromium.org/339059/diff/15001/15032
> File net/http/http_network_transaction.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15032#newcode73
> Line 73: StringAppendF(request_headers, "Content-Length: %"PRIu64"\r\n",
> Spaces.
> 
> http://codereview.chromium.org/339059/diff/15001/15033
> File net/http/partial_data.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15033#newcode241
> Line 241: headers->AddHeader(StringPrintf("%s: bytes
> %"PRId64"-%"PRId64"/%"PRId64,
> More spaces.
> 
> http://codereview.chromium.org/339059/diff/15001/15033#newcode260
> Line 260: headers->AddHeader(StringPrintf("%s: %"PRId64, kLengthHeader,
> resource_size_));
> Even more spaces.
> 
> Busted 80 columns, too.
> 
> http://codereview.chromium.org/339059/diff/15001/15040
> File webkit/glue/media/buffered_data_source_unittest.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15040#newcode96
> Line 96: "Content-Length: %"PRId64, instance_size);
> spaces.
> 
> http://codereview.chromium.org/339059/diff/15001/15040#newcode113
> Line 113: "%"PRId64"-%"PRId64"/%"PRId64,
> spaces.
> 
> http://codereview.chromium.org/339059/diff/15001/15041
> File webkit/glue/media/media_resource_loader_bridge_factory.cc (right):
> 
> http://codereview.chromium.org/339059/diff/15001/15041#newcode62
> Line 62: header = StringPrintf("Range: bytes=%"PRId64"-%"PRId64,
> spaces.
> 
> http://codereview.chromium.org/339059/diff/15001/15041#newcode67
> Line 67: header = StringPrintf("Range: bytes=%"PRId64"-",
first_byte_position);
> spaces.

When I was just done with your comments, I did the greps I should have done last
time, and noticed I again missed one of your comments from this round.  So maybe
that's why I've been doing so poorly in the past.

Running through trybots now.