<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jul 8, 2013, at 1:00 PM, Chandler Carruth wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">On Mon, Jul 8, 2013 at 12:17 PM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank" class="cremed">mren@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: mren<br>
Date: Mon Jul 8 14:17:48 2013<br>
New Revision: 185852<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=185852&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=185852&view=rev</a><br>
Log:<br>
StringRef: add DenseMapInfo for StringRef.<br></blockquote><div><br></div><div>If you're moving something into such a low level class as StringRef, please add unittests for it.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Remove the implementation in include/llvm/Support/YAMLTraits.h.<br>
Added a DenseMap type DITypeHashMap in DebugInfo.h:<br>
DenseMap<std::pair<StringRef, unsigned>, MDNode*><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/StringRef.h<br>
llvm/trunk/include/llvm/DebugInfo.h<br>
llvm/trunk/include/llvm/Support/YAMLTraits.h<br>
llvm/trunk/lib/Support/StringRef.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/StringRef.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringRef.h?rev=185852&r1=185851&r2=185852&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringRef.h?rev=185852&r1=185851&r2=185852&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ADT/StringRef.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/StringRef.h Mon Jul 8 14:17:48 2013<br>
@@ -548,6 +548,14 @@ namespace llvm {<br>
template <typename T> struct isPodLike;<br>
template <> struct isPodLike<StringRef> { static const bool value = true; };<br>
<br>
+ template <typename T> struct DenseMapInfo;<br>
+ template<> struct DenseMapInfo<StringRef> {<br>
+ static StringRef getEmptyKey() { return StringRef(); }<br>
+ static StringRef getTombstoneKey() { return StringRef(" ", 0); }<br></blockquote><div><br></div><div>This is wrong. The tombstone key and the empty key compare as equal.</div></div></div></div></blockquote><div><br></div>I moved the above from existing code in YAMLTraits and didn't carefully inspect what is being moved.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ static unsigned getHashValue(StringRef const val);<br></blockquote><div><br></div><div>No where do we put 'const' after the type, much less on a parameter type (this isn't actually a reference).</div></div></div></div></blockquote>Same here.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ static bool isEqual(StringRef const lhs,<br>
+ StringRef const rhs) { return lhs.equals(rhs); }<br>
+ };<br>
}<br>
<br>
#endif<br>
<br>
Modified: llvm/trunk/include/llvm/DebugInfo.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo.h?rev=185852&r1=185851&r2=185852&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo.h?rev=185852&r1=185851&r2=185852&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/DebugInfo.h (original)<br>
+++ llvm/trunk/include/llvm/DebugInfo.h Mon Jul 8 14:17:48 2013<br>
@@ -17,6 +17,7 @@<br>
#ifndef LLVM_DEBUGINFO_H<br>
#define LLVM_DEBUGINFO_H<br>
<br>
+#include "llvm/ADT/DenseMap.h"<br>
#include "llvm/ADT/SmallPtrSet.h"<br>
#include "llvm/ADT/SmallVector.h"<br>
#include "llvm/ADT/StringRef.h"<br>
@@ -45,6 +46,9 @@ namespace llvm {<br>
class DIType;<br>
class DIObjCProperty;<br>
<br>
+ /// Map from a pair <unique type name, an unsigned flag> to MDNode.<br>
+ typedef DenseMap<std::pair<StringRef, unsigned>, MDNode*> DITypeHashMap;<br>
+<br></blockquote><div><br></div><div>As Eric hinted at, I will say more firmly: please do not commit dead code. Save this for the patch that actually uses it.</div></div></div></div></blockquote>I was trying to move existing code from YAML to StringRef so YAML and DebugInfo can share the code.</div><div>I agree that I should not add DITypeHashMap in this patch since it is not used yet.</div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
/// DIDescriptor - A thin wraper around MDNode to access encoded debug info.<br>
/// This should not be stored in a container, because the underlying MDNode<br>
/// may change in certain situations.<br>
<br>
Modified: llvm/trunk/include/llvm/Support/YAMLTraits.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/YAMLTraits.h?rev=185852&r1=185851&r2=185852&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/YAMLTraits.h?rev=185852&r1=185851&r2=185852&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/YAMLTraits.h (original)<br>
+++ llvm/trunk/include/llvm/Support/YAMLTraits.h Mon Jul 8 14:17:48 2013<br>
@@ -760,15 +760,7 @@ private:<br>
}<br>
static inline bool classof(const MapHNode *) { return true; }<br>
<br>
- struct StrMappingInfo {<br>
- static StringRef getEmptyKey() { return StringRef(); }<br>
- static StringRef getTombstoneKey() { return StringRef(" ", 0); }<br>
- static unsigned getHashValue(StringRef const val) {<br>
- return llvm::HashString(val); }<br>
- static bool isEqual(StringRef const lhs,<br>
- StringRef const rhs) { return lhs.equals(rhs); }<br>
- };<br>
- typedef llvm::DenseMap<StringRef, HNode*, StrMappingInfo> NameToNode;<br>
+ typedef llvm::DenseMap<StringRef, HNode*> NameToNode;<br>
<br>
bool isValidKey(StringRef key);<br>
<br>
<br>
Modified: llvm/trunk/lib/Support/StringRef.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/StringRef.cpp?rev=185852&r1=185851&r2=185852&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/StringRef.cpp?rev=185852&r1=185851&r2=185852&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/StringRef.cpp (original)<br>
+++ llvm/trunk/lib/Support/StringRef.cpp Mon Jul 8 14:17:48 2013<br>
@@ -11,6 +11,7 @@<br>
#include "llvm/ADT/APInt.h"<br>
#include "llvm/ADT/Hashing.h"<br>
#include "llvm/ADT/OwningPtr.h"<br>
+#include "llvm/ADT/StringExtras.h"<br>
#include "llvm/ADT/edit_distance.h"<br>
#include <bitset><br>
<br>
@@ -465,3 +466,7 @@ bool StringRef::getAsInteger(unsigned Ra<br>
hash_code llvm::hash_value(StringRef S) {<br>
return hash_combine_range(S.begin(), S.end());<br>
}<br>
+<br>
+unsigned DenseMapInfo<StringRef>::getHashValue(StringRef const val) {<br>
+ return llvm::HashString(val);<br></blockquote><div><br></div><div>Please use hash_value here rather than HashString. HashString is optimized for identifier-like strings.</div></div></div></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div>However, this brings me to the real question I have: Why is this a good idea? We have specifically avoided the introduction of generic routines for StringRef objects in a DenseMap because for the generic case we have a much more efficient StringMap datastructure, and because there is no easy "generic" definition of both an empty and tombstone key. Why not use StringMap? I think you should provide that justification before we go any further with this line of changes to StringRef.</div></div></div></div></blockquote><div><br></div>The key here is a pair <StringRef and unsigned>. If having a generic DenseMapInfo for StringRef is not a good idea, I will introduce a DenseMapInfo for the pair and keep things where they were by reverting this patch.</div><div><br></div><div>Thanks,</div><div>Manman<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div></div></div></div>
</blockquote></div><br></body></html>