Improve EvalString

Make several improvements and fixes to `EvalString`.

  * Change the member variable to store the last text segment length (or
    0 if the last section is a variable).  This keeps things more
    understandable and doesn't lose any functionality.
  * Remove the initial `Offset` value pushed into `EvalString` in the
    constructor and `clear()` methods as this is not necessary.
  * Assert that text and variables passed in are not empty and check
    that this cannot happen (the Ninja lexer will complain that the
    file is not correct)
  * Add a natvis file for easy debugging.

CMakeLists.txt

@@ -12,6 +12,7 @@ set(CMAKE_COMPILE_WARNING_AS_ERROR ON)
 
 add_executable(
     trimja
+    src/all.natvis
     src/trimja.m.cpp
     src/allocationprofiler.cpp
     src/basicscope.cpp

src/all.natvis

@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="utf-8"?>
+<AutoVisualizer xmlns="http://schemas.microsoft.com/vstudio/debugger/natvis/2010">
+  <Type Name="trimja::EvalString">
+    <Expand>
+      <CustomListItems MaxItemsPerView="100">
+        <Variable Name="mask" InitialValue="((trimja::EvalString::Offset)1) &lt;&lt; (sizeof(trimja::EvalString::Offset) * 8 - 1)"/>
+        <Variable Name="rawLength" InitialValue="(trimja::EvalString::Offset)0"/>
+        <Variable Name="isVariable" InitialValue="false"/>
+        <Variable Name="length" InitialValue="(trimja::EvalString::Offset)0"/>
+        <Variable Name="it" InitialValue="m_data.isShortString() ? m_data._Mypair._Myval2._Bx._Buf : m_data._Mypair._Myval2._Bx._Ptr"/>
+        <Loop>
+          <Break Condition="*it == '\0'" />
+          <Exec>rawLength = *(const trimja::EvalString::Offset*)it</Exec>
+          <Exec>isVariable = (rawLength &amp; mask)</Exec>
+          <Exec>length = rawLength &amp; ~mask</Exec>
+          <If Condition="isVariable">
+            <Item Name="var">it + sizeof(trimja::EvalString::Offset),[length]s8b</Item>
+          </If>
+          <If Condition="!isVariable">
+            <Item Name="text">it + sizeof(trimja::EvalString::Offset),[length]s8</Item>
+          </If>
+          <Exec>it += length + sizeof(length)</Exec>
+        </Loop>
+      </CustomListItems>
+    </Expand>
+  </Type>
+
+  <Type Name="trimja::EvalString::const_iterator">
+    <Intrinsic Name="isEnd" Expression="*m_pos == '\0'"/>
+    <Intrinsic Name="rawLength" Expression="*(const trimja::EvalString::Offset*)m_pos"/>
+    <Intrinsic Name="mask" Expression="(trimja::EvalString::Offset)1 &lt;&lt; (sizeof(trimja::EvalString::Offset) * 8 - 1)"/>
+    <Intrinsic Name="isVariable" Expression="(rawLength() &amp; mask()) != 0"/>
+    <Intrinsic Name="length" Expression="rawLength() &amp; ~mask()"/>
+    <DisplayString Condition="isEnd()">{{ end }}</DisplayString>
+    <DisplayString Condition="!isEnd() &amp;&amp; isVariable()">{{ var = {m_pos + sizeof(trimja::EvalString::Offset),[length()]s8b} }}</DisplayString>
+    <DisplayString Condition="!isEnd() &amp;&amp; !isVariable()">{{ text = {m_pos + sizeof(trimja::EvalString::Offset),[length()]s8} }}</DisplayString>
+  </Type>
+</AutoVisualizer>

src/evalstring.cpp

@@ -28,9 +28,9 @@
 // prefixed with the length of the segment, stored as an `Offset` type.  The
 // leading bit of this is set to 1 if the segment is a variable, otherwise it is
 // 0 if the segment is text.  Additionally there is an extra member variable
-// `m_lastSegmentLength` that has the same value as the last `Offset`.  This
-// allows us to jump back to the last segment to extend it if it is a text
-// segment.
+// `m_lastTextSegmentLength` that has the length of the last text section or 0
+// if the last section was not text.  This allows us to jump back to the last
+// segment to extend it.
 //
 // This has the benefit that `EvalString` if very cache-friendly when iterating
 // and requires only one allocation.  Moves and copies should be as cheap as
@@ -101,18 +101,18 @@ bool operator!=(EvalString::const_iterator lhs,
   return !(lhs == rhs);
 }
 
-EvalString::EvalString() : m_data(sizeof(Offset), '\0'), m_lastSegmentLength{} {
+EvalString::EvalString() : m_data{}, m_lastTextSegmentLength{0} {
   assert(empty());
 }
 
 void EvalString::clear() {
-  m_data.assign(sizeof(Offset), '\0');
-  m_lastSegmentLength = 0;
+  m_data.clear();
+  m_lastTextSegmentLength = 0;
   assert(empty());
 }
 
 bool EvalString::empty() const {
-  return m_data.size() == sizeof(Offset);
+  return m_data.empty();
 }
 
 EvalString::const_iterator EvalString::begin() const {
@@ -124,27 +124,29 @@ EvalString::const_iterator EvalString::end() const {
 }
 
 void EvalString::appendText(std::string_view text) {
-  if (!hasLeadingBit(m_lastSegmentLength)) {
+  assert(!text.empty());
+  if (m_lastTextSegmentLength > 0) {
     // If the last part was plain text we can extend it
-    const Offset newLength = m_lastSegmentLength + text.size();
+    const Offset newLength = m_lastTextSegmentLength + text.size();
     std::copy_n(reinterpret_cast<const char*>(&newLength), sizeof(newLength),
-                m_data.end() - sizeof(Offset) - m_lastSegmentLength);
+                m_data.end() - sizeof(Offset) - m_lastTextSegmentLength);
     m_data.append(text);
-    m_lastSegmentLength = newLength;
+    m_lastTextSegmentLength = newLength;
   } else {
     // Otherwise write new segment
-    m_lastSegmentLength = text.size();
-    m_data.append(reinterpret_cast<const char*>(&m_lastSegmentLength),
-                  sizeof(m_lastSegmentLength));
+    const Offset length = text.size();
+    m_data.append(reinterpret_cast<const char*>(&length), sizeof(length));
     m_data.append(text);
+    m_lastTextSegmentLength = length;
   }
 }
 
 void EvalString::appendVariable(std::string_view name) {
-  m_lastSegmentLength = setLeadingBit(name.size());
-  m_data.append(reinterpret_cast<const char*>(&m_lastSegmentLength),
-                sizeof(m_lastSegmentLength));
+  assert(!name.empty());
+  const Offset length = setLeadingBit(name.size());
+  m_data.append(reinterpret_cast<const char*>(&length), sizeof(length));
   m_data.append(name);
+  m_lastTextSegmentLength = 0;
 }
 
 }  // namespace trimja

src/evalstring.h

@@ -42,7 +42,7 @@ class EvalString {
 
  private:
   std::string m_data;
-  Offset m_lastSegmentLength;
+  Offset m_lastTextSegmentLength;
 
  public:
   /**
@@ -102,13 +102,13 @@ class EvalString {
   /**
    * @brief Appends text to the EvalString, consolidating consecutive text
    * sections.
-   * @param text The text to append.
+   * @param text The text to append.  This must not be empty.
    */
   void appendText(std::string_view text);
 
   /**
    * @brief Appends a variable to the EvalString.
-   * @param name The name of the variable to append.
+   * @param name The name of the variable to append.  This must not be empty.
    */
   void appendVariable(std::string_view name);
 };

tests/variables/build.ninja

@@ -4,7 +4,7 @@ pool bye
   depth = 100
 foo = 42
 rule copy
-  command = ninja --version $in -> $out foo=$foo bar=${bar} pool=$pool
+  command = ninja --version $in -> $out foo=$foo bar=${bar}$bar pool=$pool
   pool = hi
 build out1: copy in1
   foo = 1