[PATCH] A new ADT: StringRefNulTerminated
Dmitri Gribenko
gribozavr at gmail.com
Sat Feb 23 14:50:33 PST 2013
Hi Sean,
Thank you for the review! I will post an updated patch in a few minutes.
On Thu, Feb 21, 2013 at 2:34 AM, Sean Silva <silvas at purdue.edu> wrote:
> Overall, I think this is a good idea to have.
>
> Some comments about the patch:
>
> + /// Construct a string ref from a string literal.
> + template<size_t LengthWithNul>
> + StringRefNulTerminated(const char (&Str)[LengthWithNul]):
> + StringRef(Str, LengthWithNul - 1) {}
>
> Is there a threat that this could end up being called by a non-string
> literal? If so, then it should assert. I'm thinking of a scenario like
>
> const char Foo[1] = {'a'};
> auto X = StringRefNulTerminated(Foo);
Done.
>
> + /// \brief Represents a constant reference to a NUL-terminated string.
> + /// It can be safely converted to 'const char *' without copying the
> + /// underlying data.
> + class StringRefNulTerminated : public StringRef {
> + public:
>
> This comment should really say something explaining that "it is just a
> way to represent in the type system that 'one past the end' of the
> StringRef is valid memory guaranteed to be NUL". The discussion in the
> rst documentation could be significantly simplified by describing it
> like this as well.
Changed the comment.
> + /// It can be safely converted to 'const char *' without copying the
> + /// underlying data.
>
> You should make clear that this is the motivation e.g. "The motivation
> is that it can be safely ...". I would also drop "safely" to avoid
> even suggesting the idea of looking one-past-the-end of a regular
> StringRef.
A good idea. I was using 'safely' because I was fixing that
one-past-end read, but it should not have been written.
> + StringRefNulTerminated(const char *Data, size_t Length):
> + StringRef(Data, Length) {
> + assert(Data[Length] == '\0' && "");
>
> Please make the message actually helpful. When someone crashes on this
> assert they want something meaningful.
Done. I actually intended to write the message, but apparently forgot.
--
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
More information about the llvm-commits
mailing list