Feature/string hash review (#1142)

* * Added ConstArrayView
* Made StringSlicePool have styles
* Remove point about strings not having terminating 0 (they do), and restriction around ""

* spCalcStringHash -> spComputeStringHash

* Small code improvements.
Closer to coding conventions.

* Fix small bug with Empty adding c string.

* Fix typo in assert.

* Fix ArrayView compiling issue on gcc/clang.

* Remove tabs.

* Improve comments around StringSlicePool.
Simplify getting the added slices.

3 files changed, 239 insertions, 120 deletions

diff --git a/source/core/slang-array-view.h b/source/core/slang-array-view.h
index 8b653f4c7..56c936073 100644
--- a/source/core/slang-array-view.h
+++ b/source/core/slang-array-view.h
@@ -5,108 +5,148 @@
 
 namespace Slang
 {
-	template<typename T>
-	class ArrayView
-	{
-	private:
-		T* m_buffer;
-		int m_count;
-	public:
-        const T* begin() const { return m_buffer; }
-        T* begin() { return m_buffer; }
 
+    // !!!!!!!!!!!!!!!!!!!!!!!!!!!!! ConstArrayView !!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+    template<typename T>
+    class ConstArrayView
+    {
+    public:
+        const T* begin() const { return m_buffer; }
+        
         const T* end() const { return m_buffer + m_count; }
-        T* end() { return m_buffer + m_count; }
         
-	public:
-		ArrayView():
-			m_buffer(nullptr),
-			m_count(0)
+        inline Index getCount() const { return m_count; }
+
+        inline const T& operator [](Index idx) const
+        {
+            SLANG_ASSERT(idx >= 0 && idx < m_count);
+            return m_buffer[idx];
+        }
+        
+        inline const T* getBuffer() const { return m_buffer; }
+        
+        template<typename T2>
+        Index indexOf(const T2& val) const
+        {
+            for (Index i = 0; i < m_count; i++)
+            {
+                if (m_buffer[i] == val)
+                    return i;
+            }
+            return -1;
+        }
+
+        template<typename T2>
+        Index lastIndexOf(const T2& val) const
+        {
+            for (Index i = m_count - 1; i >= 0; i--)
+            {
+                if (m_buffer[i] == val)
+                    return i;
+            }
+            return -1;
+        }
+
+        template<typename Func>
+        Index findFirstIndex(const Func& predicate) const
+        {
+            for (Index i = 0; i < m_count; i++)
+            {
+                if (predicate(m_buffer[i]))
+                    return i;
+            }
+            return -1;
+        }
+
+        template<typename Func>
+        Index findLastIndex(const Func& predicate) const
+        {
+            for (Index i = m_count - 1; i >= 0; i--)
+            {
+                if (predicate(m_buffer[i]))
+                    return i;
+            }
+            return -1;
+        }
+
+        ConstArrayView() :
+            m_buffer(nullptr),
+            m_count(0)
         {
-		}
-		ArrayView(T& singleObj):
-            m_buffer(&singleObj),
-            m_count(1)
+        }
+
+        ConstArrayView(const T* buffer, Index count) :
+            m_buffer(const_cast<T*>(buffer)),
+            m_count(count)
         {
-		}
-		ArrayView(T* buffer, int size):
+        }
+
+    protected:
+        ConstArrayView(T* buffer, Index count) :
             m_buffer(buffer),
-            m_count(size)
-		{
-		}
-		
-		inline int getCount() const { return m_count; }
-
-		inline const T& operator [](int idx) const
-		{
-            SLANG_ASSERT(idx >= 0 && idx <= m_count);
-            return m_buffer[idx];
-		}
-        inline T& operator [](int idx)
+            m_count(count)
         {
-            SLANG_ASSERT(idx >= 0 && idx <= m_count);
+        }
+
+        T* m_buffer;        ///< Note that this isn't const, as is used for derived class ArrayView also
+        Index m_count;
+    };
+
+    template<typename T>
+    ConstArrayView<T> makeConstArrayView(const T& obj)
+    {
+        return ConstArrayView<T>(&obj, 1);
+    }
+
+    template<typename T>
+    ConstArrayView<T> makeConstArrayView(const T* buffer, Index count)
+    {
+        return ConstArrayView<T>(buffer, count);
+    }
+
+    // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! ArrayView !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
+    template<typename T>
+    class ArrayView: public ConstArrayView<T>
+    {
+    public:
+        typedef ConstArrayView<T> Super;
+        
+        using Super::m_buffer;
+        using Super::m_count;
+
+        using Super::begin;
+        T* begin() { return m_buffer; }
+
+        using Super::end;
+        T* end() { return m_buffer + m_count; }
+        
+        using Super::operator[];
+        inline T& operator [](Index idx)
+        {
+            SLANG_ASSERT(idx >= 0 && idx < m_count);
             return m_buffer[idx];
         }
 
-        inline const T* getBuffer() const { return m_buffer; }
+        using Super::getBuffer;
         inline T* getBuffer() { return m_buffer; }
 
-		template<typename T2>
-		int indexOf(const T2 & val) const
-		{
-			for (int i = 0; i < m_count; i++)
-			{
-				if (m_buffer[i] == val)
-					return i;
-			}
-			return -1;
-		}
-
-		template<typename T2>
-		int lastIndexOf(const T2 & val) const
-		{
-			for (int i = m_count - 1; i >= 0; i--)
-			{
-				if (m_buffer[i] == val)
-					return i;
-			}
-			return -1;
-		}
-
-		template<typename Func>
-		int findFirstIndex(const Func& predicate) const
-		{
-			for (int i = 0; i < m_count; i++)
-			{
-				if (predicate(m_buffer[i]))
-					return i;
-			}
-			return -1;
-		}
-
-		template<typename Func>
-		int findLastIndex(const Func& predicate) const
-		{
-			for (int i = m_count - 1; i >= 0; i--)
-			{
-				if (predicate(m_buffer[i]))
-					return i;
-			}
-			return -1;
-		}
-	};
-
-	template<typename T>
-	ArrayView<T> makeArrayView(T& obj)
-	{
-		return ArrayView<T>(obj);
-	}
-		
-	template<typename T>
-	ArrayView<T> makeArrayView(T* buffer, int count)
-	{
-		return ArrayView<T>(buffer, count);
-	}
+        ArrayView() : Super() {}
+        ArrayView(T* buffer, Index size) :Super(buffer, size) {}
+    };
+
+    template<typename T>
+    ArrayView<T> makeArrayView(T& obj)
+    {
+        return ArrayView<T>(&obj, 1);
+    }
+        
+    template<typename T>
+    ArrayView<T> makeArrayView(T* buffer, Index count)
+    {
+        return ArrayView<T>(buffer, count);
+    }
 }
 
 #endif
diff --git a/source/core/slang-string-slice-pool.cpp b/source/core/slang-string-slice-pool.cpp
index e3d9d8809..7ea15e0d6 100644
--- a/source/core/slang-string-slice-pool.cpp
+++ b/source/core/slang-string-slice-pool.cpp
@@ -5,10 +5,10 @@ namespace Slang {
 /* static */ const StringSlicePool::Handle StringSlicePool::kNullHandle;
 /* static */ const StringSlicePool::Handle StringSlicePool::kEmptyHandle;
 
-/* static */const int StringSlicePool::kNumDefaultHandles;
+/* static */const Index StringSlicePool::kDefaultHandlesCount;
 
-
-StringSlicePool::StringSlicePool() :
+StringSlicePool::StringSlicePool(Style style) :
+    m_style(style),
     m_arena(1024)
 {
     clear();
@@ -16,15 +16,29 @@ StringSlicePool::StringSlicePool() :
 
 void StringSlicePool::clear()
 {
-    m_slices.setCount(2);
-
-    m_slices[0] = UnownedStringSlice((const char*)nullptr, (const char*)nullptr);
-    m_slices[1] = UnownedStringSlice::fromLiteral("");
-    
-    // Add the empty entry
-    m_map.Add(m_slices[1], kEmptyHandle);
-
     m_map.Clear();
+
+    switch (m_style)
+    {
+        case Style::Default:
+        {
+            // Add the defaults
+            m_slices.setCount(2);
+
+            m_slices[0] = UnownedStringSlice((const char*)nullptr, (const char*)nullptr);
+            m_slices[1] = UnownedStringSlice::fromLiteral("");
+            
+            // Add the empty entry
+            m_map.Add(m_slices[1], kEmptyHandle);
+            break;
+        }
+        case Style::Empty:
+        {
+            // There are no defaults
+            m_slices.clear();
+            break;
+        }
+    }
 }
 
 StringSlicePool::Handle StringSlicePool::add(const Slice& slice)
@@ -47,31 +61,53 @@ StringSlicePool::Handle StringSlicePool::add(const Slice& slice)
 
 StringSlicePool::Handle StringSlicePool::add(StringRepresentation* stringRep)
 {
-    if (stringRep == nullptr)
+    if (stringRep == nullptr && m_style == Style::Default)
     {
         return kNullHandle;
     }
     return add(StringRepresentation::asSlice(stringRep));
 }
  
-
 StringSlicePool::Handle StringSlicePool::add(const char* chars)
 {
-    if (!chars)
+    switch (m_style)
     {
-        return kNullHandle;
-    }
-    if (chars[0] == 0)
-    {
-        return kEmptyHandle;
+        case Style::Default:
+        {
+            if (!chars)
+            {
+                return kNullHandle;
+            }
+            if (chars[0] == 0)
+            {
+                return kEmptyHandle;
+            }
+            break;
+        }
+        case Style::Empty:
+        {
+            if (chars == nullptr)
+            {
+                SLANG_ASSERT(!"Empty style doesn't support nullptr");
+                // Return an invalid handle
+                return Handle(~HandleIntegral(0));
+            }
+        }
     }
+    
     return add(UnownedStringSlice(chars));
 }
 
-int StringSlicePool::findIndex(const Slice& slice) const
+Index StringSlicePool::findIndex(const Slice& slice) const
 {
     const Handle* handlePtr = m_map.TryGetValue(slice);
-    return handlePtr ? int(*handlePtr) : -1;
+    return handlePtr ? Index(*handlePtr) : -1;
+}
 
+ConstArrayView<UnownedStringSlice> StringSlicePool::getAdded() const
+{
+    const Index firstIndex = getFirstAddedIndex();
+    return makeConstArrayView(m_slices.getBuffer() + firstIndex, m_slices.getCount() - firstIndex);
 }
+
 } // namespace Slang
diff --git a/source/core/slang-string-slice-pool.h b/source/core/slang-string-slice-pool.h
index 4d5f91e37..2e4b41333 100644
--- a/source/core/slang-string-slice-pool.h
+++ b/source/core/slang-string-slice-pool.h
@@ -6,24 +6,52 @@
 #include "slang-list.h"
 #include "slang-memory-arena.h"
 #include "slang-dictionary.h"
+#include "slang-array-view.h"
 
 namespace Slang {
 
+/* Holds a unique set of slices.
+
+Note that all slices (except kNullHandle) are stored with terminating zeros.
+
+The default handles kNullHandle, kEmptyHandle can only be used on a StringSlicePool
+initialized with the Style::Default. Not doing so will return an undefined result.
+
+TODO(JS): 
+An argument could be made to make different classes, perhaps deriving from a base class
+that exhibited the two behaviors. That doing so would make the default handles defined
+for that class for example.
+
+This is a little awkward in practice, because behavior of some methods need to change
+(like adding a c string with nullptr, or clearing, as well as some other perhaps less necessary
+optimizations). This could be achieved via virtual functions, but this all seems overkill. 
+*/
 class StringSlicePool
 {
 public:
+    typedef StringSlicePool ThisType;
+    typedef uint32_t HandleIntegral;
+
+    enum class Style
+    {
+        Default,            ///< Default style - has default handles (like kNullHandle and kEmptyHandle)
+        Empty,              ///< Empty style - has no handles by default. Using default handles will likely produce the wrong result. 
+    };
 
-    /// Handle of 0 is null. If accessed will be returned as the empty string
-    enum class Handle : uint32_t;
+    enum class Handle : HandleIntegral;
     typedef UnownedStringSlice Slice;
 
+        /// The following default handles *only* apply if constructed with the Style::Default
+
+        /// Handle of 0 is null. If accessed will be returned as the empty string with nullptr the chars
     static const Handle kNullHandle = Handle(0);
+        /// Handle of 1 is the empty string. 
     static const Handle kEmptyHandle = Handle(1);
 
-    static const int kNumDefaultHandles = 2;
+    static const Index kDefaultHandlesCount = 2;
 
         /// Returns the index of a slice, if contained, or -1 if not found
-    int findIndex(const Slice& slice) const;
+    Index findIndex(const Slice& slice) const;
 
         /// True if has the slice
     bool has(const Slice& slice) { return findIndex(slice) >= 0; }
@@ -46,17 +74,32 @@ public:
     const List<UnownedStringSlice>& getSlices() const { return m_slices; }
 
         /// Get the number of slices
-    int getNumSlices() const { return int(m_slices.getCount()); }
+    Index getSlicesCount() const { return m_slices.getCount(); }
+
+        /// Returns true if the handle is a default one. Only meaningful on a Style::Default.
+    bool isDefaultHandle(Handle handle) const { SLANG_ASSERT(m_style == Style::Default && Index(handle) >= 0); return Index(handle) < kDefaultHandlesCount; }
 
         /// Convert a handle to and index. (A handle is just an index!) 
-    static int asIndex(Handle handle) { return int(handle); }
-        /// Returns true if the handle is to a slice that contains characters (ie not null or empty)
-    static bool hasContents(Handle handle) { return int(handle) >= kNumDefaultHandles; }
+    static Index asIndex(Handle handle) { return Index(handle); }
+    
+        /// Get the style of the pool
+    Style getStyle() const { return m_style; }
+
+        /// Get all the added slices (does not have default slices, if there are any)
+    ConstArrayView<UnownedStringSlice> getAdded() const; 
+
+        /// Get the index of the first added handle
+    Index getFirstAddedIndex() const { return m_style == Style::Default ? kDefaultHandlesCount : 0; }
 
         /// Ctor
-    StringSlicePool();
+    explicit StringSlicePool(Style style);
 
 protected:
+    // Disable copy ctor and assignment
+    StringSlicePool(const ThisType& rhs) = delete;
+    void operator=(const ThisType& rhs) = delete;
+
+    Style m_style;
     List<UnownedStringSlice> m_slices;
     Dictionary<UnownedStringSlice, Handle> m_map;
     MemoryArena m_arena;