Add shared macros for stringizing and converting ANSI string constants...

Add shared macros for stringizing and converting ANSI string constants
(in particular defined by macros) to wide string constants.

Convert existing locally-defined stringizing to use the shared macros.

Unit tests for the shared macros.

This also fixes a minor bug in ceee_module_util.cc where I
accidentally quoted a string constant I only meant to convert to wide
(this caused no bug, but was unintended, so the change in semantics
in that file in the current change is intentional).

BUG=none
TEST=automated tests



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66579

[Jói, 2010-11-17 00:28:12]

Reviewers:
- Evan for base
- Andrew for media\base\media_posix.cc
- Dean for net\base\net_errors.cc

[Jói, 2010-11-17 16:40:23]

ping

On Tue, Nov 16, 2010 at 7:28 PM,  <joi@chromium.org> wrote:
> Reviewers: Evan Martin, scherkus, Dean McNamee,
>
> Message:
> Reviewers:
> - Evan for base
> - Andrew for media\base\media_posix.cc
> - Dean for net\base\net_errors.cc
>
> Description:
> Add shared macros for stringizing and converting ANSI string constants
> (in particular defined by macros) to wide string constants.
>
> Convert existing locally-defined stringizing to use the shared macros.
>
> Unit tests for the shared macros.
>
> This also fixes a minor bug in ceee_module_util.cc where I
> accidentally quoted a string constant I only meant to convert to wide
> (this caused no bug, but was unintended, so the change in semantics
> in that file in the current change is intentional).
>
> BUG=none
> TEST=automated tests
>
>
>
> Please review this at http://codereview.chromium.org/5103001/
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     base/base.gyp
>  M     base/base.gypi
>  A     base/preprocessor_util.h
>  A     base/preprocessor_util_unittest.cc
>  M     ceee/ie/common/ceee_module_util.cc
>  M     media/base/media_posix.cc
>  M     net/base/net_errors.cc
>
>
>

[Evan Martin, 2010-11-17 23:37:29]

Sorry for losing your change.  :(

Regarding the file names -- we have a few other files that are also preprocessor
utilities (see compiler_specific.h, format_macros.h).  Why not name them closer
to their purpose, to something like stringize_macros.h?

http://codereview.chromium.org/5103001/diff/1/base/preprocessor_util.h
File base/preprocessor_util.h (right):

http://codereview.chromium.org/5103001/diff/1/base/preprocessor_util.h#newcode15
base/preprocessor_util.h:15: #define LSTRINGIZE_NO_EXPANSION(x) L ## #x
We have deprecated wide strings.  If you really need these for some
Windows-specific code, can you wrap them in an #if defined(OS_WIN) block so that
people aren't tempted to use them elsewhere?

[Evan Martin, 2010-11-17 23:38:33]

If the L-string versions are really only used by one file, maybe they belong in
that file.

[Jói, 2010-11-17 23:40:40]

Thanks Evan. I expect the L-string versions will be needed in more
places and would prefer that people use a central, tested version.

Will wrap the L versions in #if defined (OS_WIN) and rename the file
as you suggested.

Cheers,
Jói


On Wed, Nov 17, 2010 at 6:38 PM,  <evan@chromium.org> wrote:
> If the L-string versions are really only used by one file, maybe they belong
> in
> that file.
>
> http://codereview.chromium.org/5103001/
>

[Jói, 2010-11-18 00:12:30]

PTAL, I uploaded a new version with the changes we discussed.

On 2010/11/17 23:40:40, Jói wrote:
> Thanks Evan. I expect the L-string versions will be needed in more
> places and would prefer that people use a central, tested version.
> 
> Will wrap the L versions in #if defined (OS_WIN) and rename the file
> as you suggested.
> 
> Cheers,
> Jói
> 
> 
> On Wed, Nov 17, 2010 at 6:38 PM,  <mailto:evan@chromium.org> wrote:
> > If the L-string versions are really only used by one file, maybe they belong
> > in
> > that file.
> >
> > http://codereview.chromium.org/5103001/
> >

[Evan Martin, 2010-11-18 00:15:37]

great, LGTM (some things to check below)

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h
File base/stringize_macros.h (right):

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h#new...
base/stringize_macros.h:16: #define STRINGIZE_NO_EXPANSION(x) #x            \
Unintentional trailing backslash (?)

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
File base/stringize_macros_unittest.cc (right):

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
base/stringize_macros_unittest.cc:33: #if defined(OS_WIN)
Double check that this code is running on Windows -- you might need to include
build/build_config.h

[Jói, 2010-11-18 00:26:44]

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h
File base/stringize_macros.h (right):

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h#new...
base/stringize_macros.h:16: #define STRINGIZE_NO_EXPANSION(x) #x            \
On 2010/11/18 00:15:37, Evan Martin wrote:
> Unintentional trailing backslash (?)

Whoops, removed.

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
File base/stringize_macros_unittest.cc (right):

http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
base/stringize_macros_unittest.cc:33: #if defined(OS_WIN)
On 2010/11/18 00:15:37, Evan Martin wrote:
> Double check that this code is running on Windows -- you might need to include
> build/build_config.h

It runs on my Windows box; do you feel I should add an include of
build/build_config.h anyway, as belt and suspenders?

[Jói, 2010-11-18 00:44:34]

>> Double check that this code is running on Windows -- you might need to
>
> include
>>
>> build/build_config.h
>
> It runs on my Windows box; do you feel I should add an include of
> build/build_config.h anyway, as belt and suspenders?

Scratch that, you were right, I was building from a stale .sln file.
Added the build/build_config.h include.

Waiting for Andrew to LGTM and the tree to open up.

Cheers,
Jói



On Wed, Nov 17, 2010 at 7:26 PM,  <joi@chromium.org> wrote:
>
> http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h
> File base/stringize_macros.h (right):
>
>
http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros.h#new...
> base/stringize_macros.h:16: #define STRINGIZE_NO_EXPANSION(x) #x
>   \
> On 2010/11/18 00:15:37, Evan Martin wrote:
>>
>> Unintentional trailing backslash (?)
>
> Whoops, removed.
>
>
http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
> File base/stringize_macros_unittest.cc (right):
>
>
http://codereview.chromium.org/5103001/diff/20001/base/stringize_macros_unitt...
> base/stringize_macros_unittest.cc:33: #if defined(OS_WIN)
> On 2010/11/18 00:15:37, Evan Martin wrote:
>>
>> Double check that this code is running on Windows -- you might need to
>
> include
>>
>> build/build_config.h
>
> It runs on my Windows box; do you feel I should add an include of
> build/build_config.h anyway, as belt and suspenders?
>
> http://codereview.chromium.org/5103001/
>

[scherkus (not reviewing), 2010-11-18 01:24:54]

LGTM!

I think it's right -- and if it's not we'll know rather quickly ;)