Travis Vitek wrote:
[...]
Martin Sebor wrote:
A few questions/comments on the implementation:
http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/src/braceexp.cpp?r=628817
The _rw_is_lower() and _rw_is_upper() helper functions should either
be replaced with the C equivalents or rewritten to handle EBCDIC. If
the latter, I suggest making them inline.
The C equivalents are documented to be locale dependent. I don't believe
that we want the input string to be localized, so I've written my own
routines that are equivalent the the C functions for the classic locale.
Don't forget about EBCDIC where letters of the alphabet don't have
consecutive values. See src/ctype.cpp.
I'm not sure why you suggest making them inline. Is there some other reason
other than the obvious, and possibly unnecessary, optimization benefit?
Just optimization.
Martin Sebor wrote:
In _rw_brace_graph, the member functions don't need to be privatized
(with the _C_ prefix). Also, unless the class is intended to be copy
constructible and assignable I suggest to explicitly declare the two
members private.
Actually, none of this needs to be privatized because it is already private
to the implementation. Yay.
Martin Sebor wrote:
Finally, please check your braces :) E.g., these:
http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/src/braceexp.cpp?r=628817#l29
http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/src/braceexp.cpp?r=628817#l54
http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/src/braceexp.cpp?r=628817#l87
Have I mentioned that our brace convention is wonky? ;-)
Hey, at least it's consistent (for the most part ;-)
Martin
