Martin Sebor wrote:
>
> This is super cool! I've played with it a bit and except for a few
> minor issues (mostly with ranges) it works like a charm! (I've just
> committed an enhanced version of the test that reveals these problems.)
>
Yes, thank you for your enhancements to the test suite. As you've probably
seen, I've already responded to those additions in another thread. You can
find that discussion here [http://tinyurl.com/3xnkve].
About the interface:
http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/include/rw_braceexp.h?r=628611
Can the third argument be negative? (If not, it might make sense
to change its type to size_t).
No, it should not be. Even if it were accepted it should be treated as 0.
Martin Sebor wrote:
>
> What about the sep argument? Can
> it be outside of the range of char, such as -1? It seems to just
> get converted to char here (oooh, line number links ;-):
> http://fisheye6.cenqua.com/browse/stdcxx/trunk/tests/src/braceexp.cpp?r=628817#l274
> It might make sense to do some basic checking on it (and on the
> other arguments) in rw_brace_expand() before passing it to the
> implementation.
>
No, sep should be convertable to an unsigned char, just as it should be when
you call 'strchr'.
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.
I'm not sure why you suggest making them inline. Is there some other reason
other than the obvious, and possibly unnecessary, optimization benefit?
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? ;-)
Travis
--
View this message in context:
http://www.nabble.com/Re%3A-svn-commit%3A-r628611---in--stdcxx-trunk-tests%3A-include-rw_braceexp.h-self-0.braceexp.cpp-src-braceexp.cpp-tp15552138p15562361.html
Sent from the stdcxx-dev mailing list archive at Nabble.com.
