From b11aaa16d404ce7dc55de6b7338dccbf053a72bd Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 28 Jul 2016 07:17:29 +0200
Subject: Adjust to new path implementation, use to support reversibility

---
 build2/config/operation.cxx             |  2 +-
 build2/context                          | 12 +++----
 build2/context.cxx                      | 34 +++++---------------
 build2/dist/operation.cxx               |  2 +-
 build2/dump.cxx                         |  1 +
 build2/install/rule.cxx                 |  4 +--
 build2/name                             | 41 +++++++++++-------------
 build2/name.ixx                         | 10 ------
 build2/parser.cxx                       | 55 +++++++++++++--------------------
 build2/scope                            |  2 +-
 build2/types                            |  7 ++---
 build2/utility.cxx                      | 22 ++-----------
 build2/variable                         | 18 +++++------
 build2/variable.cxx                     | 24 +++++++-------
 build2/variable.ixx                     | 11 ++++---
 tests/test.sh                           |  4 ++-
 tests/variable/dir-path/buildfile       | 45 +++++++++++++++++++++++++++
 tests/variable/dir-path/test.out        | 22 +++++++++++++
 tests/variable/dir-path/test.sh         |  3 ++
 tests/variable/representation/buildfile |  3 ++
 tests/variable/representation/test.sh   |  4 +++
 21 files changed, 167 insertions(+), 159 deletions(-)
 create mode 100644 tests/variable/dir-path/buildfile
 create mode 100644 tests/variable/dir-path/test.out
 create mode 100755 tests/variable/dir-path/test.sh
 create mode 100755 tests/variable/representation/test.sh

diff --git a/build2/config/operation.cxx b/build2/config/operation.cxx
index 59d5349..faeb570 100644
--- a/build2/config/operation.cxx
+++ b/build2/config/operation.cxx
@@ -52,7 +52,7 @@ namespace build2
         ofs << "# Created automatically by the config module." << endl
             << "#" << endl
             << "src_root = ";
-        to_stream (ofs, name (src_root), true, '@'); // Quote.
+        to_stream (ofs, name (src_root, false), true, '@'); // Quote.
         ofs << endl;
 
         ofs.close ();
diff --git a/build2/context b/build2/context
index 93cf5ad..e7b8c7d 100644
--- a/build2/context
+++ b/build2/context
@@ -101,16 +101,12 @@ namespace build2
   extern const dir_path* relative_base;
 
   // In addition to calling relative(), this function also uses shorter
-  // notations such as '~/'.
+  // notations such as '~/'. For directories the result includes the trailing
+  // slash. If the path is the same as base, returns "./" if current is true
+  // and empty string otherwise.
   //
   string
-  diag_relative (const path&);
-
-  // As above but also adds trailing '/'. If the path is the same as
-  // base, returns "./" if current is true and empty string otherwise.
-  //
-  string
-  diag_relative (const dir_path&, bool current = true);
+  diag_relative (const path&, bool current = true);
 
   // Action phrases, e.g., "configure update exe{foo}", "updating exe{foo}",
   // and "updating exe{foo} is configured". Use like this:
diff --git a/build2/context.cxx b/build2/context.cxx
index f756bfc..e50a74a 100644
--- a/build2/context.cxx
+++ b/build2/context.cxx
@@ -368,7 +368,7 @@ namespace build2
   const dir_path* relative_base = &work;
 
   string
-  diag_relative (const path& p)
+  diag_relative (const path& p, bool cur)
   {
     if (p.string () == "-")
       return "<stdin>";
@@ -378,11 +378,11 @@ namespace build2
     if (p.absolute ())
     {
       if (p == b)
-        return ".";
+        return cur ? "." + p.separator_string () : string ();
 
 #ifndef _WIN32
       if (p == home)
-        return "~";
+        return "~" + p.separator_string ();
 #endif
 
       path rb (relative (p));
@@ -396,36 +396,18 @@ namespace build2
         if (p.sub (home))
         {
           path rh (p.leaf (home));
-          if (rb.string ().size () > rh.string ().size () + 2) // 2 for '~/'
-            return "~/" + rh.string ();
+          if (rb.size () > rh.size () + 2) // 2 for '~/'
+            return "~/" + move (rh).representation ();
         }
       }
       else if (rb.sub (home))
-        return "~/" + rb.leaf (home).string ();
+        return "~/" + rb.leaf (home).representation ();
 #endif
 
-      return rb.string ();
+      return move (rb).representation ();
     }
 
-    return p.string ();
-  }
-
-  string
-  diag_relative (const dir_path& d, bool cur)
-  {
-    string r (diag_relative (static_cast<const path&> (d)));
-
-    // Translate "." to empty.
-    //
-    if (!cur && d.absolute () && r == ".")
-      r.clear ();
-
-    // Add trailing '/'.
-    //
-    if (!r.empty () && !dir_path::traits::is_separator (r.back ()))
-      r += '/';
-
-    return r;
+    return p.representation ();
   }
 
   // diag_do(), etc.
diff --git a/build2/dist/operation.cxx b/build2/dist/operation.cxx
index 4697c6c..c87f0e7 100644
--- a/build2/dist/operation.cxx
+++ b/build2/dist/operation.cxx
@@ -338,7 +338,7 @@ namespace build2
     static void
     install (const path& cmd, file& t, const dir_path& d)
     {
-      path reld (relative (d));
+      dir_path reld (relative (d));
       path relf (relative (t.path ()));
 
       cstrings args {cmd.string ().c_str ()};
diff --git a/build2/dump.cxx b/build2/dump.cxx
index 7cb97ef..a8b02d1 100644
--- a/build2/dump.cxx
+++ b/build2/dump.cxx
@@ -69,6 +69,7 @@ namespace build2
     {
       // @@ Might be useful to dump the cache.
       //
+      assert (org->type == nullptr);
       os << var.name << (org->extra == 1 ? " =+ " : " += ");
     }
     else
diff --git a/build2/install/rule.cxx b/build2/install/rule.cxx
index 9157413..33356b0 100644
--- a/build2/install/rule.cxx
+++ b/build2/install/rule.cxx
@@ -239,7 +239,7 @@ namespace build2
     static void
     install (const install_dir& base, const dir_path& d)
     {
-      path reld (relative (d));
+      dir_path reld (relative (d));
 
       cstrings args;
 
@@ -288,7 +288,7 @@ namespace build2
     static void
     install (const install_dir& base, file& t, bool verbose = true)
     {
-      path reld (relative (base.dir));
+      dir_path reld (relative (base.dir));
       path relf (relative (t.path ()));
 
       cstrings args;
diff --git a/build2/name b/build2/name
index 84e334a..4e514c5 100644
--- a/build2/name
+++ b/build2/name
@@ -16,16 +16,21 @@ namespace build2
 {
   using std::move;
 
-  // A name is what we operate on by default. Depending on the context,
-  // it can be interpreted as a target or prerequisite name. A name
-  // without a type and directory can be used to represent any text.
-  // A name with directory and empty value represents a directory.
-  // A name may also be project-qualified. If the project name is
-  // empty, then it means the name is in a project other than our
-  // own (e.g., it is installed).
+  // A name is what we operate on by default. Depending on the context, it can
+  // be interpreted as a target or prerequisite name. A name without a type
+  // and directory can be used to represent any text. A name with directory
+  // and empty value represents a directory. A name may also be qualified with
+  // a project. If the project name is empty, then it means the name is in a
+  // project other than our own (e.g., it is installed).
   //
-  // If pair is not '\0', then this name and the next in the list
-  // form a pair.
+  // If pair is not '\0', then this name and the next in the list form a
+  // pair. Can be used as a bool flag.
+  //
+  // The original flag indicates whether this is the original name (e.g., came
+  // from the buildfile) or if it is the result of reversing a typed value.
+  // Original names have stricter representation requirements since we don't
+  // know what they actually mean (e.g., is s/foo/bar/ really a directory or
+  // a sed script).
   //
   struct name
   {
@@ -33,17 +38,12 @@ namespace build2
     dir_path dir;
     string type;
     string value;
-    char pair = '\0'; // Pair character if first half of a pair. Can be used
-                      // as bool.
+    char pair = '\0';
+    bool original = true;
 
     name () = default;
-
-    explicit name (string v): value (move (v)) {}
-    name& operator= (string v) {return *this = name (move (v));}
-
-    explicit name (dir_path d): dir (move (d)) {}
-    name& operator= (dir_path d) {return *this = name (move (d));}
-
+    name (string v, bool o): value (move (v)), original (o) {}
+    name (dir_path d, bool o): dir (move (d)), original (o) {}
     name (string t, string v): type (move (t)), value (move (v)) {}
 
     name (dir_path d, string t, string v)
@@ -90,11 +90,6 @@ namespace build2
 
     int
     compare (const name&) const;
-
-    // The result is an unqualified, simple empty name.
-    //
-    void
-    clear ();
   };
 
   inline bool
diff --git a/build2/name.ixx b/build2/name.ixx
index 73dbb4a..e2eb3ba 100644
--- a/build2/name.ixx
+++ b/build2/name.ixx
@@ -29,14 +29,4 @@ namespace build2
 
     return r;
   }
-
-  inline void name::
-  clear ()
-  {
-    proj = nullptr;
-    dir.clear ();
-    type.clear ();
-    value.clear ();
-    pair = '\0';
-  }
 }
diff --git a/build2/parser.cxx b/build2/parser.cxx
index a64a94e..ce31042 100644
--- a/build2/parser.cxx
+++ b/build2/parser.cxx
@@ -803,8 +803,7 @@ namespace build2
         p /= path ("buildfile");
       else
       {
-        bool d (path::traits::is_separator (n.value.back ())
-                || n.type == "dir");
+        bool d (path::traits::is_separator (n.value.back ()));
 
         p /= path (move (n.value));
         if (d)
@@ -1933,6 +1932,12 @@ namespace build2
     return count;
   }
 
+  // Slashe(s) plus '%'. Note that here we assume '/' is there since that's
+  // in our buildfile "syntax".
+  //
+  static const string name_separators (
+    string (path::traits::directory_separators) + '%');
+
   bool parser::
   names (token& t, type& tt,
          names_type& ns,
@@ -2011,9 +2016,9 @@ namespace build2
           continue;
         }
 
-        //@@ PATH
+        // Find slash or %.
         //
-        string::size_type p (name.find_last_of ("/\\%"));
+        string::size_type p (name.find_last_of (name_separators));
 
         // First take care of project. A project-qualified name is
         // not very common, so we can afford some copying for the
@@ -2115,30 +2120,6 @@ namespace build2
         //
         if (p == n)
         {
-          // @@ PATH: currently we treat backslashes as directory separators
-          //    which means they will be reversed as forward slashes. We have
-          //    no choice here since otherwise Windows paths will be pretty
-          //    much unusable (e.g., configure(C:\foo\@...)). Some ideas on
-          //    how we could mitigate this:
-          //
-          //    - Only do this in buildspec parsing (but what about command
-          //      line vars).
-          //
-          //    - When reversing, employ some heuristics to decide which
-          //      slash to use. E.g., if it already contains back slashes.
-          //      Though the problem is with reversing something that isn't
-          //      a path, say 'foo\'.
-          //
-
-          // Take care of "/" and "...//" (the latter is not reversible).
-          //
-          char t ('\0');
-          if (p != 0 && !path::traits::is_separator (name[p - 1]))
-          {
-            t = name[p];
-            name.resize (p); // Strip trailing '/'.
-          }
-
           // For reversibility to simple name, only treat it as a directory
           // if the string is an exact representation.
           //
@@ -2155,11 +2136,6 @@ namespace build2
                              string ());
             continue;
           }
-
-          // Add the trailing slash back and treat it as a simple name.
-          //
-          if (t != '\0')
-            name.push_back (t);
         }
 
         ns.emplace_back (pp1,
@@ -2257,6 +2233,9 @@ namespace build2
 
           if (tt == type::lparen)
           {
+            // Function call.
+            //
+
             next (t, tt); // Get '('.
 
             // Just a stub for now.
@@ -2300,6 +2279,9 @@ namespace build2
           }
           else
           {
+            // Variable expansion.
+            //
+
             // Process variable name.
             //
             if (name.front () == '.') // Fully qualified name.
@@ -2404,7 +2386,11 @@ namespace build2
             if (!n.value.empty ())
               fail (loc) << "concatenating " << what << " contains directory";
 
-            concat += n.dir.string ();
+            // If this is an original name, then we cannot assume it is
+            // actually a directory path (think s/foo/bar/) so we have to
+            // reverse it exactly.
+            //
+            concat += n.original ? n.dir.representation () : n.dir.string ();
           }
           else
             concat += n.value;
@@ -2474,6 +2460,7 @@ namespace build2
                              n.value);
 
             ns.back ().pair = n.pair;
+            ns.back ().original = n.original;
           }
 
           count = lv.size ();
diff --git a/build2/scope b/build2/scope
index 70eb73d..2efa899 100644
--- a/build2/scope
+++ b/build2/scope
@@ -308,7 +308,7 @@ namespace build2
       // directory (i.e., the calling code does not know what it is dealing
       // with), so let's use the whole path.
       //
-      return find (dir_path (p.string ()));
+      return find (path_cast<dir_path> (p));
     }
   };
 
diff --git a/build2/types b/build2/types
index fcfdcef..5409c60 100644
--- a/build2/types
+++ b/build2/types
@@ -96,21 +96,18 @@ namespace build2
     using dir_path::dir_path;
 
     explicit
-    abs_dir_path (dir_path d): dir_path (std::move (d).string (), false) {}
+    abs_dir_path (dir_path d): dir_path (std::move (d)) {}
     abs_dir_path () = default;
   };
 
   using paths = std::vector<path>;
   using dir_paths = std::vector<dir_path>;
 
-  // Custom path printing (implemented in utility.cxx).
+  // Path printing with trailing slash for directories (utility.cxx).
   //
   ostream&
   operator<< (ostream&, const path&);
 
-  ostream&
-  operator<< (ostream&, const dir_path&);
-
   // <butl/timestamp>
   //
   using butl::system_clock;
diff --git a/build2/utility.cxx b/build2/utility.cxx
index c43d048..87a2749 100644
--- a/build2/utility.cxx
+++ b/build2/utility.cxx
@@ -22,25 +22,9 @@ namespace build2
   ostream&
   operator<< (ostream& os, const path& p)
   {
-    return os << (stream_verb (os) < 2 ? diag_relative (p) : p.string ());
-  }
-
-  ostream&
-  operator<< (ostream& os, const dir_path& d)
-  {
-    if (stream_verb (os) < 2)
-      os << diag_relative (d); // Adds trailing '/'.
-    else
-    {
-      const string& s (d.string ());
-
-      // Print the directory with trailing '/'.
-      //
-      if (!s.empty ())
-        os << s << (dir_path::traits::is_separator (s.back ()) ? "" : "/");
-    }
-
-    return os;
+    return os << (stream_verb (os) < 2
+                  ? diag_relative (p)
+                  : p.representation ());
   }
 
   //
diff --git a/build2/variable b/build2/variable
index e2dc943..f3fc0dd 100644
--- a/build2/variable
+++ b/build2/variable
@@ -279,10 +279,10 @@ namespace build2
   void typify (value&, const variable&);
   void typify (value&, const value_type&, const variable*);
 
-  // Reverse the value back to names. The value should no be NULL and storage
+  // Reverse the value back to names. The value should not be NULL and storage
   // should be empty.
   //
-  names_view
+  vector_view<const name>
   reverse (const value&, names& storage);
 
   vector_view<name>
@@ -474,7 +474,7 @@ namespace build2
     static bool convert (name&&, name*);
     static void assign (value&, bool);
     static void append (value&, bool); // OR.
-    static name reverse (bool x) {return name (x ? "true" : "false");}
+    static name reverse (bool x) {return name (x ? "true" : "false", false);}
     static int compare (bool, bool);
 
     static const char* const type_name;
@@ -489,7 +489,7 @@ namespace build2
     static uint64_t convert (name&&, name*);
     static void assign (value&, uint64_t);
     static void append (value&, uint64_t); // ADD.
-    static name reverse (uint64_t x) {return name (to_string (x));}
+    static name reverse (uint64_t x) {return name (to_string (x), false);}
     static int compare (uint64_t, uint64_t);
 
     static const char* const type_name;
@@ -507,7 +507,7 @@ namespace build2
     static void assign (value&, string&&);
     static void append (value&, string&&);
     static void prepend (value&, string&&);
-    static name reverse (const string& x) {return name (x);}
+    static name reverse (const string& x) {return name (x, false);}
     static int compare (const string&, const string&);
     static bool empty (const string& x) {return x.empty ();}
 
@@ -526,7 +526,7 @@ namespace build2
     static void assign (value&, path&&);
     static void append (value&, path&&);  // operator/
     static void prepend (value&, path&&); // operator/
-    static name reverse (const path& x) {return name (x.string ());}
+    static name reverse (const path& x) {return name (x.string (), false);}
     static int compare (const path&, const path&);
     static bool empty (const path& x) {return x.empty ();}
 
@@ -545,7 +545,7 @@ namespace build2
     static void assign (value&, dir_path&&);
     static void append (value&, dir_path&&);  // operator/
     static void prepend (value&, dir_path&&); // operator/
-    static name reverse (const dir_path& x) {return name (x);}
+    static name reverse (const dir_path& x) {return name (x, false);}
     static int compare (const dir_path&, const dir_path&);
     static bool empty (const dir_path& x) {return x.empty ();}
 
@@ -564,7 +564,7 @@ namespace build2
     static abs_dir_path convert (name&&, name*);
     static void assign (value&, abs_dir_path&&);
     static void append (value&, abs_dir_path&&);  // operator/
-    static name reverse (const abs_dir_path& x) {return name (x);}
+    static name reverse (const abs_dir_path& x) {return name (x, false);}
     static int compare (const abs_dir_path&, const abs_dir_path&);
     static bool empty (const abs_dir_path& x) {return x.empty ();}
 
@@ -581,8 +581,6 @@ namespace build2
 
     static name convert (name&&, name*);
     static void assign (value&, name&&);
-    static void append (value&, name&&);
-    static void prepend (value&, name&&);
     static name reverse (const name& x) {return x;}
     static int compare (const name&, const name&);
     static bool empty (const name& x) {return x.empty ();}
diff --git a/build2/variable.cxx b/build2/variable.cxx
index 7a333bb..2d4db86 100644
--- a/build2/variable.cxx
+++ b/build2/variable.cxx
@@ -436,16 +436,13 @@ namespace build2
     string s;
 
     if (n.directory (true))
-    {
-      s = move (n.dir).string (); // Move string out of path.
-
-      // Add / back to the end of the path unless it is already there. Note
-      // that the string cannot be empty (n.directory () would have been
-      // false).
+      // Use either the precise or traditional representation depending on
+      // whethe this is the original name (if it is, then this might not be
+      // a path after all; think s/foo/bar/).
       //
-      if (!dir_path::traits::is_separator (s[s.size () - 1]))
-        s += '/';
-    }
+      s = n.original
+        ? move (n.dir).representation () // Move out of path.
+        : move (n.dir).string ();
     else
       s.swap (n.value);
 
@@ -473,10 +470,10 @@ namespace build2
 
       if (r->directory (true))
       {
-        s += r->dir.string ();
-
-        if (!dir_path::traits::is_separator (s[s.size () - 1]))
-          s += '/';
+        if (r->original)
+          s += move (r->dir).representation ();
+        else
+          s += r->dir.string ();
       }
       else
         s += r->value;
@@ -636,6 +633,7 @@ namespace build2
     if (r != nullptr)
       throw invalid_argument (string ());
 
+    n.original = false;
     return move (n);
   }
 
diff --git a/build2/variable.ixx b/build2/variable.ixx
index 6b59c72..4b699a6 100644
--- a/build2/variable.ixx
+++ b/build2/variable.ixx
@@ -189,7 +189,7 @@ namespace build2
       typify (v, t, &var);
   }
 
-  inline names_view
+  inline vector_view<const name>
   reverse (const value& v, names& storage)
   {
     assert (v &&
@@ -463,10 +463,11 @@ namespace build2
   inline void value_traits<name>::
   assign (value& v, name&& x)
   {
-    if (v)
-      v.as<name> () = move (x);
-    else
-      new (&v.data_) name (move (x));
+    name* p (v
+             ? &(v.as<name> () = move (x))
+             : new (&v.data_) name (move (x)));
+
+    p->original = false;
   }
 
   inline int value_traits<name>::
diff --git a/tests/test.sh b/tests/test.sh
index b26dd3b..f29b43a 100755
--- a/tests/test.sh
+++ b/tests/test.sh
@@ -7,7 +7,7 @@ export PATH=$cur_dir/../build2:$PATH
 
 function test ()
 {
-  echo "Testing $1"
+  echo "testing $1"
   cd "$cur_dir/$1"
   ./test.sh
 }
@@ -22,10 +22,12 @@ test "names"
 test "pairs"
 test "quote"
 test "scope"
+test "variable/dir-path"
 test "variable/expansion"
 test "variable/null"
 test "variable/override"
 test "variable/prepend"
 test "variable/qualified"
+test "variable/representation"
 test "variable/type"
 test "variable/type-pattern-append"
diff --git a/tests/variable/dir-path/buildfile b/tests/variable/dir-path/buildfile
new file mode 100644
index 0000000..5c99f07
--- /dev/null
+++ b/tests/variable/dir-path/buildfile
@@ -0,0 +1,45 @@
+# Untyped dir path reversability.
+#
+x = s/foo/bar/
+print -e=$x
+print -e $x
+
+y = $x
+print -e=$y
+print -e $y
+print
+
+# Typed dir path reversability and expansion.
+#
+x = [dir_path] foo/bar/
+print -I$x
+print -I$x/baz
+print -I $x
+print [strings] -I $x
+print -I $x/baz
+print
+
+y = $x             # No longer typed but still not original.
+print -I$y
+print -I$y/baz
+print -I $y
+print [strings] -I $y
+print -I $y/baz
+print
+
+z = [strings] $x   # Re-typed.
+print $z
+print
+
+# The root case.
+#
+r = [dir_path] /
+print $r/foo
+
+r += foo
+print [strings] $r
+
+r += bar
+print [strings] $r
+
+./:
diff --git a/tests/variable/dir-path/test.out b/tests/variable/dir-path/test.out
new file mode 100644
index 0000000..e608c42
--- /dev/null
+++ b/tests/variable/dir-path/test.out
@@ -0,0 +1,22 @@
+-e=s/foo/bar/
+-e s/foo/bar/
+-e=s/foo/bar/
+-e s/foo/bar/
+
+-Ifoo/bar
+-Ifoo/bar/baz
+-I foo/bar/
+-I foo/bar
+-I foo/bar/baz
+
+-Ifoo/bar
+-Ifoo/bar/baz
+-I foo/bar/
+-I foo/bar
+-I foo/bar/baz
+
+foo/bar
+
+//foo
+/foo
+/foo/bar
diff --git a/tests/variable/dir-path/test.sh b/tests/variable/dir-path/test.sh
new file mode 100755
index 0000000..c745b76
--- /dev/null
+++ b/tests/variable/dir-path/test.sh
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+b -q | diff --strip-trailing-cr -u test.out -
diff --git a/tests/variable/representation/buildfile b/tests/variable/representation/buildfile
index 9bb90d1..b9c213d 100644
--- a/tests/variable/representation/buildfile
+++ b/tests/variable/representation/buildfile
@@ -1,3 +1,6 @@
+# @@ I wonder if we can redo this test in terms of print?
+#
+
 # Test reversibility of variable representation.
 #
 val  = -L/
diff --git a/tests/variable/representation/test.sh b/tests/variable/representation/test.sh
new file mode 100755
index 0000000..da9e0e9
--- /dev/null
+++ b/tests/variable/representation/test.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+b test
+
-- 
cgit v1.1