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-----
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.
line 53: it seems that this doesn't restore the
float field, base field etc.
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.
line 41: From the way domains is used, would a std::set
be more appropriate?
(Issues like this will be caught by
the inspect tool, so not a big deal now.)
line 30 and others,: casting to and from void* should use
line 56: can you use invalid instead of hard-coding
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?
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.
line 123: I take it that the format guarantees that
hash_size_ > 2? Please validate this requirement
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
line 407: This isn't guaranteed to work. The
standard doesn't guarantee that the letters
are contiguous. (Does anything use EBCDIC
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
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.
line 239: There's no 'c' in tokenizer.
Everything except compile can be in the unnamed namespace.
Is this header intended to be used in multiple
translation units? If so you probably should
make the functions inline and not use an
line 49: Don't most Windows functions use DWORD,
rather than int?
line 48: I think I just saw code looking just
like this in another file.
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.
line 32: I think I saw another copy of these functions.
Get rid of the duplication?
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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
-----END PGP SIGNATURE-----
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk