Re: [boost] [locale] review part 2.2: source

Subject: Re: [boost] [locale] review part 2.2: source
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-04-14 23:24:41

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

AMDG

shared/date_time.cpp:

line 79: If it's an invariant that tz_ is
  the same as the timezone of impl, then
  this is not exception safe if clone throws.
  You'll have to use copy and swap idiom.

line 161: This is a bit odd, don't you think?  Normally
  if assignment is strongly exception safe without
  doing any extra work, there's no reason to go out
  of your way to clear the old state on failure.

line 350: should you check that v is in range?
  Regardless, the valid range should be documented.

shared/format.cpp:

line 53: it seems that this doesn't restore the
  float field, base field etc.

shared/formatting.cpp:

line 42: normally copy and swap is the preferred
  way to implement assignment.  The main
  advantage is that it guarantees that
  if the operation fails, the state of
  the lhs is unchanged.

shared/generator.hpp:

line 41: From the way domains is used, would a std::set
  be more appropriate?

shared/ids.cpp:

missing copyright/license.
(Issues like this will be caught by
the inspect tool, so not a big deal now.)

shared/ios_prop.hpp:

line 30 and others,: casting to and from void* should use
  static_cast.

line 56: can you use invalid instead of hard-coding
  this constant?

line 96: I don't think this is guaranteed to
  be distinct from any valid pointer.  Can
  you make invalid the address of a unique
  global object instead?

shared/localization_backend.cpp:

lines 200, 206: This is not guaranteed to be thread-safe
  in C++03.  You must use call_once for thread-safe
  static inititalization.  Note however that this is
  probably safe if you assume that no additional
  threads are started before main.

shared/message.cpp:

line 123: I take it that the format guarantees that
  hash_size_ > 2?  Please validate this requirement
  when loading.

line 132: Use null_pair?

line 152: You require that off is in range, that
  off <= off + len, and that off + len is in range.
  Please check this.

line 204: You really should check that you actually read
  4 bytes.  Otherwise if the file is too short, you get
  undefined behavior from reading uninitialized memory.

line 311: How do you know that there's a null-terminator?

line 326: suppose that form is 0 and the null-terminator
  is missing...

line 407: This isn't guaranteed to work.  The
  standard doesn't guarantee that the letters
  are contiguous.  (Does anything use EBCDIC
  these days?)

lines 440, 442: You're assuming that the string is
  null terminated.  What if the file is incorrectly
  formatted?  It may be possible to read past
  the end of the allocated block.  I haven't
  actually worked through the exact code path
  to trigger this condition, but it seems suspicious.

line 475: You're assuming that the separator
  actually appears.

line 500: I think this can cause alignment problems.
  Does the file format make any alignment guarantees?
  Even if it does, you'll need to handle invalid
  files gracefully.  Of course, it'll only be
  inefficient on platforms that allow unaligned
  loads, but still, the standard doesn't guarantee
  that it'll work.

shared/mo_hash.hpp:

shared/mo_lambda.cpp:

line 239: There's no 'c' in tokenizer.

Everything except compile can be in the unnamed namespace.

shared/mo_lambda.hpp:

std/all_generater.hpp:

std/codecvt.cpp:

std/collate.cpp:

std/converter.cpp:

std/numeric.cpp:

std/std_backend.cpp:

std/std_backend.hpp:

util/codecvt_converter.cpp:

util/default_locale.cpp:

util/iconv.hpp:

Is this header intended to be used in multiple
translation units?  If so you probably should
make the functions inline and not use an
unnamed namespace.

util/info.hpp:

util/locale_data.hpp:

util/locale_data.cpp:

util/numeric.cpp:

util/timezone.hpp:

win32/all_generator.hpp:

win32/api.hpp:

line 49: Don't most Windows functions use DWORD,
  rather than int?

win32/collate.hpp:

line 48: I think I just saw code looking just
  like this in another file.

win32/converter.cpp:

win32/lcid.cpp:

line 51: Check whether this succeeds and uses
  the entire string?

line 89: I thought double checked locking was
  considered unsafe?  Anyway you should use
  call_once for initialization like this.

win32/lcid.hpp:

win32/numeric.cpp:

line 32: I think I saw another copy of these functions.
  Get rid of the duplication?

win32/win_backend.cpp:

win32/win_backend.hpp:

build/Jamfile.v2:

line 30: I don't think you need a separate
  object file.  It isn't used anywhere else.

line 126: A simpler way to reference another
  library is /boost//thread

line 211: I would tend to use
  icu-sources = [ glob icu/*.cpp ] ;
  result += <source>$(icu-sources)
  I see you're using something like
  this later on.

line 278: Boost.Thread ought to set this flag
  automatically.  I'm assuming from the fact
  that you have it, that it doesn't.  Looks
  like a bug in the thread Jamfile.  (Similarly,
  you should add <define>BOOST_LOCALE_NO_LIB
  in the usage requirements of boost_locale)

test/Jamfile.v2: You don't actually need
  test-suite.  test-suite is exactly the
  same as alias.

In Christ,
Steven Watanabe
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNp7p5AAoJEDTBQuhymLHSiv4H/0HCgXhXiOAsMmMFY6BYn9W7
aSKX3n9qGCDTG7WI+tHoZwz46e6BHrf7PlfcA+6Z1v4wA1GBonxxXrmTN8AhNIIm
ozn7Qlw7xH13rO7r3BfPPmib0sSOWCTEUcIfCI2ZW4huKuGMOnmI+tLaHr6qwVP3
+xAiWjAzNkShzn8Qy2rWYo9X+mYaXBrBjViejlyn7M0ArW2Rgcsu8FPWh8Kkhzqk
6TcltpHmUI4dQ9uAkJhP9zcjqyKIUA9vTApcKCDJMqGnAwSDoXA+ZWZGuoquhsia
4BAkL32+E9EnpEf982R+3cQ7NqYQy452rB9TC02qsFrYbrPv6FKDmQMb0uWvBGM=
=bzjH
-----END PGP SIGNATURE-----