[llvm] r185852 - StringRef: add DenseMapInfo for StringRef.

[Manman Ren]

On Jul 8, 2013, at 1:00 PM, Chandler Carruth wrote:

> On Mon, Jul 8, 2013 at 12:17 PM, Manman Ren <mren at apple.com> wrote:
> Author: mren
> Date: Mon Jul  8 14:17:48 2013
> New Revision: 185852
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=185852&view=rev
> Log:
> StringRef: add DenseMapInfo for StringRef.
> 
> If you're moving something into such a low level class as StringRef, please add unittests for it.
> 
> 
> Remove the implementation in include/llvm/Support/YAMLTraits.h.
> Added a DenseMap type DITypeHashMap in DebugInfo.h:
>   DenseMap<std::pair<StringRef, unsigned>, MDNode*>
> 
> Modified:
>     llvm/trunk/include/llvm/ADT/StringRef.h
>     llvm/trunk/include/llvm/DebugInfo.h
>     llvm/trunk/include/llvm/Support/YAMLTraits.h
>     llvm/trunk/lib/Support/StringRef.cpp
> 
> Modified: llvm/trunk/include/llvm/ADT/StringRef.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringRef.h?rev=185852&r1=185851&r2=185852&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/StringRef.h (original)
> +++ llvm/trunk/include/llvm/ADT/StringRef.h Mon Jul  8 14:17:48 2013
> @@ -548,6 +548,14 @@ namespace llvm {
>    template <typename T> struct isPodLike;
>    template <> struct isPodLike<StringRef> { static const bool value = true; };
> 
> +  template <typename T> struct DenseMapInfo;
> +  template<> struct DenseMapInfo<StringRef> {
> +    static StringRef getEmptyKey() { return StringRef(); }
> +    static StringRef getTombstoneKey() { return StringRef(" ", 0); }
> 
> This is wrong. The tombstone key and the empty key compare as equal.

I moved the above from existing code in YAMLTraits and didn't carefully inspect what is being moved.
>  
> +    static unsigned getHashValue(StringRef const val);
> 
> No where do we put 'const' after the type, much less on a parameter type (this isn't actually a reference).
Same here.
>  
> +    static bool isEqual(StringRef const lhs,
> +                        StringRef const rhs) { return lhs.equals(rhs); }
> +  };
>  }
> 
>  #endif
> 
> Modified: llvm/trunk/include/llvm/DebugInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo.h?rev=185852&r1=185851&r2=185852&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/DebugInfo.h (original)
> +++ llvm/trunk/include/llvm/DebugInfo.h Mon Jul  8 14:17:48 2013
> @@ -17,6 +17,7 @@
>  #ifndef LLVM_DEBUGINFO_H
>  #define LLVM_DEBUGINFO_H
> 
> +#include "llvm/ADT/DenseMap.h"
>  #include "llvm/ADT/SmallPtrSet.h"
>  #include "llvm/ADT/SmallVector.h"
>  #include "llvm/ADT/StringRef.h"
> @@ -45,6 +46,9 @@ namespace llvm {
>    class DIType;
>    class DIObjCProperty;
> 
> +  /// Map from a pair <unique type name, an unsigned flag> to MDNode.
> +  typedef DenseMap<std::pair<StringRef, unsigned>, MDNode*> DITypeHashMap;
> +
> 
> As Eric hinted at, I will say more firmly: please do not commit dead code. Save this for the patch that actually uses it.
I was trying to move existing code from YAML to StringRef so YAML and DebugInfo can share the code.
I agree that I should not add DITypeHashMap in this patch since it is not used yet.

>  
>    /// DIDescriptor - A thin wraper around MDNode to access encoded debug info.
>    /// This should not be stored in a container, because the underlying MDNode
>    /// may change in certain situations.
> 
> Modified: llvm/trunk/include/llvm/Support/YAMLTraits.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/YAMLTraits.h?rev=185852&r1=185851&r2=185852&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/YAMLTraits.h (original)
> +++ llvm/trunk/include/llvm/Support/YAMLTraits.h Mon Jul  8 14:17:48 2013
> @@ -760,15 +760,7 @@ private:
>      }
>      static inline bool classof(const MapHNode *) { return true; }
> 
> -    struct StrMappingInfo {
> -      static StringRef getEmptyKey() { return StringRef(); }
> -      static StringRef getTombstoneKey() { return StringRef(" ", 0); }
> -      static unsigned getHashValue(StringRef const val) {
> -                                                return llvm::HashString(val); }
> -      static bool isEqual(StringRef const lhs,
> -                          StringRef const rhs) { return lhs.equals(rhs); }
> -    };
> -    typedef llvm::DenseMap<StringRef, HNode*, StrMappingInfo> NameToNode;
> +    typedef llvm::DenseMap<StringRef, HNode*> NameToNode;
> 
>      bool isValidKey(StringRef key);
> 
> 
> Modified: llvm/trunk/lib/Support/StringRef.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/StringRef.cpp?rev=185852&r1=185851&r2=185852&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/StringRef.cpp (original)
> +++ llvm/trunk/lib/Support/StringRef.cpp Mon Jul  8 14:17:48 2013
> @@ -11,6 +11,7 @@
>  #include "llvm/ADT/APInt.h"
>  #include "llvm/ADT/Hashing.h"
>  #include "llvm/ADT/OwningPtr.h"
> +#include "llvm/ADT/StringExtras.h"
>  #include "llvm/ADT/edit_distance.h"
>  #include <bitset>
> 
> @@ -465,3 +466,7 @@ bool StringRef::getAsInteger(unsigned Ra
>  hash_code llvm::hash_value(StringRef S) {
>    return hash_combine_range(S.begin(), S.end());
>  }
> +
> +unsigned DenseMapInfo<StringRef>::getHashValue(StringRef const val) {
> +  return llvm::HashString(val);
> 
> Please use hash_value here rather than HashString. HashString is optimized for identifier-like strings.
>  
> 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.

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.

Thanks,
Manman
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130708/4d2a3cfe/attachment.html>