<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Apr 20, 2017 at 9:00 PM Greg Clayton via Phabricator via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org">lldb-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg requested changes to this revision.<br>
clayborg added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
Very close. A few misuses of ConstString and this will be good to go.<br>
<br>
<br>
<br>
================<br>
Comment at: include/lldb/Interpreter/Property.h:43<br>
+ ConstString GetName() const { return m_name; }<br>
+ ConstString GetDescription() const { return m_description; }<br>
<br>
----------------<br>
This shouldn't be const-ified, Only names of things like variables, enumerators, typenames, paths, and other strings that are going to be uniqued should go into the ConstStringPool<br>
<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ObjectFile.h:569<br>
/// Some object files may have an identifier string embedded in them,<br>
- /// e.g. in a Mach-O core file using the LC_IDENT load command (which<br>
+ /// e.g. in a Mach-O core file using the LC_IDENT load command (which<br>
/// is obsolete, but can still be found in some old files)<br>
----------------<br>
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.<br>
<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ObjectFile.h:576-577<br>
//------------------------------------------------------------------<br>
- virtual std::string GetIdentifierString () {<br>
- return std::string();<br>
+ virtual std::string GetIdentifierString () {<br>
+ return std::string();<br>
}<br>
----------------<br>
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.<br>
<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ObjectFile.h:583<br>
/// "binary" in the core file. lldb can iterate over the pages looking<br>
- /// for a valid binary, but some core files may have metadata<br>
+ /// for a valid binary, but some core files may have metadata<br>
/// describing where the main binary is exactly which removes ambiguity<br>
----------------<br>
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.<br>
<br>
<br>
================<br>
Comment at: include/lldb/Symbol/ObjectFile.h:808-811<br>
+ virtual ConstString<br>
+ StripLinkerSymbolAnnotations(ConstString symbol_name) const {<br>
+ return symbol_name;<br>
}<br>
----------------<br>
This actually doesn't need to change. Since it is merely stripping off parts of the string, this should really stay as a StringRef and it should return a StringRef. Change to use StringRef for return and for argument, or revert.<br>
<br>
<br>
<br>
================<br>
Comment at: include/lldb/Utility/ConstString.h:481<br>
+ char operator[](size_t Index) const {<br>
+ assert(Index < GetLength() && "Invalid index!");<br>
+ return m_string[Index];<br>
----------------<br>
I really don't like the prospect of crashing the debugger if someone calls this with an invalid index. Many people ship with asserts on. I would either make it safe if it is indeed just for reporting or remove the assert.<br></blockquote><div>Every other string implementation behaves the same way. No different than if you have a raw char array and you access it out of bounds. If you just remove the assert, then you're not going to find out about this until you crash off in la-la land after a heap corruption and it will be significantly harder to diagnose.</div><div><br></div><div>In any case, I don't see a way to make it safe either. If someone accesses an array out of its bounds, what can you really do? People shouldn't be doing that, and if they do we should fail immediately.</div></div></div>