<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 17, 2013 at 6:48 PM, José Fonseca <span dir="ltr"><<a href="mailto:jose.r.fonseca@gmail.com" target="_blank">jose.r.fonseca@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="im">On Mon, Jun 17, 2013 at 6:17 PM, Alexander Monakov <span dir="ltr"><<a href="mailto:amonakov@ispras.ru" target="_blank">amonakov@ispras.ru</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>> It would be nice to have an assert to truncate(ssize_t len) method that<br>
> `find('\0') < buffer[len`], to catch similar cases to creep in the future.<br>><br>> Could you send me an updated patch?<br><br></div>In the meantime I've sent the patch with the readlink error fixed in a<br>
different way to the list. Now truncate(len) is not used anywhere. <br></blockquote><div><br></div></div><div>I see no point in calling strlen() when we already know the length beforehand.</div><div class="im"><div><br>
</div><div>> Please add `assert(strlen(str()) == length);` in the end of truncate(size_t) in a separate commit if you want.</div>
</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Will do.</div><div class="gmail_quote"><div class="im"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Overall I find this os::String class rather confusing to new and future<br>apitrace developers. Are we supposed to use it for all strings, or should we<br>use std::string where possible?</blockquote><div><br></div></div>
<div>
Use std::string where possible. The os::String's class comment is terse but clear (though misspelled):</div><div><br></div><div><div> /**</div><div> * Vector based zero-terminate string, suitable for passing strings or paths</div>
<div> * to/from OS calls.</div><div> */</div></div><div><br></div><div>The crucial issue here is that both Win32 and POSIX return strings as zero length buffers, but std::string only provides read-only C strings. So there is no way to get the output of OS calls into a std::strings without using temporary mallocs, which seemed awful to me, especially because ultimately the strings would be passed back to OS which would again expect a zero terminator.<br>
</div><div><br></div><div>I googled on this issue and the general advice online seemed to be that if you need to manipulate zero terminated strings then you're better off using std::vector<char> instead std::string, hence os::String was born.</div>
<div><br></div><div>I suppose additional comments don't hurt, so I'll add these paragraphs as well.</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Why was os::Path name decided less suitable?<br></blockquote><div><br></div></div><div>Process names are not stricly paths, and I expected I need to use it for things like arguments, environment variables, etc -- most OS calls that return strings, even those that don't deal with paths, usually expect a sized buffer where to write it into.</div>
<div class="im">
<div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">And this truncate() method is a bit odd.<span><font color="#888888"><br>
</font></span></blockquote><div> </div></div><div>I'm not sure what you mean. The only odd thing I can see is the absence of a comment, but I'll fix that.</div><div><br></div></div></div></div></blockquote><div><br>
</div><div style>Don't know if this is what you meant, but indeed the str() call in `truncate(strlen(str())) ` indeed is not correct, as the assertion inside str() may fail erroneously.</div><div style><br></div><div style>
Jose</div></div><br></div></div>