From 53f02bf28dae507a51515ed6ac03226d68816494 Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 2 Feb 2017 10:20:50 +0200
Subject: Store extension in target map key rather than in target

This is in preparation for locking its modification/access.
---
 build2/bin/target.cxx   | 42 ++++++++++++------------
 build2/cc/compile.cxx   |  2 +-
 build2/cc/link.cxx      |  2 +-
 build2/cc/pkgconfig.cxx |  4 +--
 build2/cli/rule.cxx     | 11 ++++---
 build2/cli/target       |  4 +--
 build2/cli/target.cxx   |  4 +--
 build2/prerequisite.cxx |  2 +-
 build2/scope.cxx        |  6 ++--
 build2/target           | 23 +++++++------
 build2/target-key       |  4 +--
 build2/target-type      | 12 ++++---
 build2/target.cxx       | 87 ++++++++++++++++++++++++++-----------------------
 build2/test/target.cxx  |  5 +--
 14 files changed, 110 insertions(+), 98 deletions(-)

diff --git a/build2/bin/target.cxx b/build2/bin/target.cxx
index e9ef81e..ba15c7d 100644
--- a/build2/bin/target.cxx
+++ b/build2/bin/target.cxx
@@ -12,7 +12,7 @@ namespace build2
   {
     extern const char ext_var[] = "extension"; // VC14 rejects constexpr.
 
-    static target*
+    static pair<target*, optional<string>>
     obje_factory (const target_type&,
                   dir_path dir,
                   dir_path out,
@@ -20,12 +20,12 @@ namespace build2
                   optional<string> ext)
     {
       obj* o (targets.find<obj> (dir, out, n));
-      obje* e (new obje (move (dir), move (out), move (n), move (ext)));
+      obje* e (new obje (move (dir), move (out), move (n)));
 
       if ((e->group = o))
         o->e = e;
 
-      return e;
+      return make_pair (e, move (ext));
     }
 
     const target_type obje::static_type
@@ -39,7 +39,7 @@ namespace build2
       false
     };
 
-    static target*
+    static pair<target*, optional<string>>
     obja_factory (const target_type&,
                   dir_path dir,
                   dir_path out,
@@ -47,12 +47,12 @@ namespace build2
                   optional<string> ext)
     {
       obj* o (targets.find<obj> (dir, out, n));
-      obja* a (new obja (move (dir), move (out), move (n), move (ext)));
+      obja* a (new obja (move (dir), move (out), move (n)));
 
       if ((a->group = o))
         o->a = a;
 
-      return a;
+      return make_pair (a, move (ext));
     }
 
     const target_type obja::static_type
@@ -66,7 +66,7 @@ namespace build2
       false
     };
 
-    static target*
+    static pair<target*, optional<string>>
     objs_factory (const target_type&,
                    dir_path dir,
                    dir_path out,
@@ -74,12 +74,12 @@ namespace build2
                    optional<string> ext)
     {
       obj* o (targets.find<obj> (dir, out, n));
-      objs* s (new objs (move (dir), move (out), move (n), move (ext)));
+      objs* s (new objs (move (dir), move (out), move (n)));
 
       if ((s->group = o))
         o->s = s;
 
-      return s;
+      return make_pair (s, move (ext));
     }
 
     const target_type objs::static_type
@@ -93,7 +93,7 @@ namespace build2
       false
     };
 
-    static target*
+    static pair<target*, optional<string>>
     obj_factory (const target_type&,
                  dir_path dir,
                  dir_path out,
@@ -104,7 +104,7 @@ namespace build2
       obja* a (targets.find<obja> (dir, out, n));
       objs* s (targets.find<objs> (dir, out, n));
 
-      obj* o (new obj (move (dir), move (out), move (n), move (ext)));
+      obj* o (new obj (move (dir), move (out), move (n)));
 
       if ((o->e = e))
         e->group = o;
@@ -115,7 +115,7 @@ namespace build2
       if ((o->s = s))
         s->group = o;
 
-      return o;
+      return make_pair (o, move (ext));
     }
 
     const target_type obj::static_type
@@ -129,7 +129,7 @@ namespace build2
       false
     };
 
-    static target*
+    static pair<target*, optional<string>>
     liba_factory (const target_type& t,
                   dir_path d,
                   dir_path o,
@@ -139,12 +139,12 @@ namespace build2
       // Only link-up to the group if the types match exactly.
       //
       lib* l (t == liba::static_type ? targets.find<lib> (d, o, n) : nullptr);
-      liba* a (new liba (move (d), move (o), move (n), move (ext)));
+      liba* a (new liba (move (d), move (o), move (n)));
 
       if ((a->group = l))
         l->a = a;
 
-      return a;
+      return make_pair (a, move (ext));
     }
 
     // @@
@@ -170,7 +170,7 @@ namespace build2
       false
     };
 
-    static target*
+    static pair<target*, optional<string>>
     libs_factory (const target_type& t,
                   dir_path d,
                   dir_path o,
@@ -180,12 +180,12 @@ namespace build2
       // Only link-up to the group if the types match exactly.
       //
       lib* l (t == libs::static_type ? targets.find<lib> (d, o, n) : nullptr);
-      libs* s (new libs (move (d), move (o), move (n), move (ext)));
+      libs* s (new libs (move (d), move (o), move (n)));
 
       if ((s->group = l))
         l->s = s;
 
-      return s;
+      return make_pair (s, move (ext));
     }
 
     const target_type libs::static_type
@@ -210,7 +210,7 @@ namespace build2
       // members to implement "library meta-information protocol".
     }
 
-    static target*
+    static pair<target*, optional<string>>
     lib_factory (const target_type&,
                  dir_path d,
                  dir_path o,
@@ -220,7 +220,7 @@ namespace build2
       liba* a (targets.find<liba> (d, o, n));
       libs* s (targets.find<libs> (d, o, n));
 
-      lib* l (new lib (move (d), move (o), move (n), move (ext)));
+      lib* l (new lib (move (d), move (o), move (n)));
 
       if ((l->a = a))
         a->group = l;
@@ -228,7 +228,7 @@ namespace build2
       if ((l->s = s))
         s->group = l;
 
-      return l;
+      return make_pair (l, move (ext));
     }
 
     const target_type lib::static_type
diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx
index 2176b82..1fed762 100644
--- a/build2/cc/compile.cxx
+++ b/build2/cc/compile.cxx
@@ -422,7 +422,7 @@ namespace build2
         // only use the target type and name from the target key so we can
         // pass bogus values for the rest.
         //
-        target_key tk {&tt, nullptr, nullptr, &n, target_key::nullext};
+        target_key tk {&tt, nullptr, nullptr, &n, nullopt};
 
         // This is like prerequisite search.
         //
diff --git a/build2/cc/link.cxx b/build2/cc/link.cxx
index d85faed..7798c95 100644
--- a/build2/cc/link.cxx
+++ b/build2/cc/link.cxx
@@ -549,7 +549,7 @@ namespace build2
           }
 
           if (pt == nullptr)
-            pt = &search (ott, o.dir, o.out, o.name, o.ext, nullptr);
+            pt = &search (ott, o.dir, o.out, o.name, o.ext (), nullptr);
         }
         else
           pt = &ot;
diff --git a/build2/cc/pkgconfig.cxx b/build2/cc/pkgconfig.cxx
index d32cd61..da6614f 100644
--- a/build2/cc/pkgconfig.cxx
+++ b/build2/cc/pkgconfig.cxx
@@ -419,9 +419,7 @@ namespace build2
           string name (l, 2); // Sans -l.
 
           prerequisite_key pk {
-            nullopt,
-            {&lib::static_type, &out, &out, &name, target_key::nullext},
-            &s};
+            nullopt, {&lib::static_type, &out, &out, &name, nullopt}, &s};
 
           if (lib* lt = static_cast<lib*> (search_library (sysd, usrd, pk)))
           {
diff --git a/build2/cli/rule.cxx b/build2/cli/rule.cxx
index 773d00c..262e89a 100644
--- a/build2/cli/rule.cxx
+++ b/build2/cli/rule.cxx
@@ -211,17 +211,20 @@ namespace build2
                       const char* option,
                       const char* default_extension)
     {
-      assert (t.ext); // Should have been figured out in apply().
+      // Should have been figured out in apply().
+      //
+      const optional<string>& e (*t.ext_);
+      assert (e);
 
-      if (*t.ext != default_extension)
+      if (*e != default_extension)
       {
         // CLI needs the extension with the leading dot (unless it is empty)
         // while we store the extension without. But if there is an extension,
         // then we can get it (with the dot) from the file name.
         //
         args.push_back (option);
-        args.push_back (t.ext->empty ()
-                        ? t.ext->c_str ()
+        args.push_back (e->empty ()
+                        ? e->c_str ()
                         : t.path ().extension_cstring () - 1);
       }
     }
diff --git a/build2/cli/target b/build2/cli/target
index 0cc448d..f4ab7e7 100644
--- a/build2/cli/target
+++ b/build2/cli/target
@@ -49,8 +49,8 @@ namespace build2
 
       //using mtime_target::mtime_target; // @@ GCC 4.8
 
-      cli_cxx (dir_path d, dir_path o, string n, optional<string> e)
-          : mtime_target (move (d), move (o), move (n), move (e))
+      cli_cxx (dir_path d, dir_path o, string n)
+          : mtime_target (move (d), move (o), move (n))
       {
         m[0] = m[1] = m[2] = nullptr;
       }
diff --git a/build2/cli/target.cxx b/build2/cli/target.cxx
index 3f09d30..48dc95b 100644
--- a/build2/cli/target.cxx
+++ b/build2/cli/target.cxx
@@ -48,7 +48,7 @@ namespace build2
       return file_mtime (h->path ());
     }
 
-    static target*
+    static pair<target*, optional<string>>
     cli_cxx_factory (const target_type&,
                      dir_path d,
                      dir_path o,
@@ -66,7 +66,7 @@ namespace build2
       targets.insert<cxx::cxx> (d, o, n, trace);
       targets.insert<cxx::ixx> (d, o, n, trace);
 
-      return new cli_cxx (move (d), move (o), move (n), move (e));
+      return make_pair (new cli_cxx (move (d), move (o), move (n)), move (e));
     }
 
     const target_type cli_cxx::static_type
diff --git a/build2/prerequisite.cxx b/build2/prerequisite.cxx
index 3bffa3d..ac89456 100644
--- a/build2/prerequisite.cxx
+++ b/build2/prerequisite.cxx
@@ -57,7 +57,7 @@ namespace build2
         dir (t.dir),
         out (t.out),   // @@ If it's empty, then we treat as undetermined?
         name (t.name),
-        ext (t.ext),
+        ext (t.ext ()),
         scope (t.base_scope ()),
         target (&t)
   {
diff --git a/build2/scope.cxx b/build2/scope.cxx
index 0c80052..0b76647 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -621,7 +621,7 @@ namespace build2
     return r;
   }
 
-  static target*
+  static pair<target*, optional<string>>
   derived_tt_factory (const target_type& t,
                       dir_path d,
                       dir_path o,
@@ -639,8 +639,8 @@ namespace build2
     const target_type* bt (t.base);
     for (; bt->factory == &derived_tt_factory; bt = bt->base) ;
 
-    target* r (bt->factory (t, move (d), move (o), move (n), move (e)));
-    r->derived_type = &t;
+    auto r (bt->factory (t, move (d), move (o), move (n), move (e)));
+    r.first->derived_type = &t;
     return r;
   }
 
diff --git a/build2/target b/build2/target
index 05e4281..42c0361 100644
--- a/build2/target
+++ b/build2/target
@@ -108,7 +108,7 @@ namespace build2
   class target
   {
   public:
-    typedef build2::action action_type;
+    using action_type = build2::action;
 
     // For targets that are in the src tree of a project we also keep the
     // corresponding out directory. As a result we may end up with multiple
@@ -127,7 +127,9 @@ namespace build2
     const dir_path   dir;  // Absolute and normalized.
     const dir_path   out;  // Empty or absolute and normalized.
     const string     name;
-    optional<string> ext;  // Absent - unspecified, empty - no extension.
+    optional<string> ext () const {return *ext_;}
+
+    optional<string>* ext_; // Reference to value in target_key.
 
     const dir_path&
     out_dir () const {return out.empty () ? dir : out;}
@@ -249,7 +251,7 @@ namespace build2
     // to the targets's members will be reflected in the key.
     //
     target_key
-    key () const {return target_key {&type (), &dir, &out, &name, ext};}
+    key () const {return target_key {&type (), &dir, &out, &name, ext ()};}
 
     // Scoping.
     //
@@ -548,8 +550,10 @@ namespace build2
     // The only way to create a target should be via the targets set below.
     //
   public:
-    target (dir_path d, dir_path o, string n, optional<string> e)
-        : dir (move (d)), out (move (o)), name (move (n)), ext (move (e)),
+    friend class target_set;
+
+    target (dir_path d, dir_path o, string n)
+        : dir (move (d)), out (move (o)), name (move (n)),
           vars (false) // Note: not global.
     {}
   };
@@ -967,10 +971,9 @@ namespace build2
   // map. The key's hash ignores the extension, so the hash will stay stable
   // across extension updates.
   //
-  struct target_set
+  class target_set
   {
-    // @@ Why do we dynalloc target?
-    //
+  public:
     typedef std::unordered_map<target_key, unique_ptr<target>> map;
     typedef butl::map_iterator_adapter<map::const_iterator> iterator;
 
@@ -1342,14 +1345,14 @@ namespace build2
   // search functions.
   //
   template <typename T>
-  target*
+  pair<target*, optional<string>>
   target_factory (const target_type&,
                   dir_path d,
                   dir_path o,
                   string n,
                   optional<string> e)
   {
-    return new T (move (d), move (o), move (n), move (e));
+    return make_pair (new T (move (d), move (o), move (n)), move (e));
   }
 
   // Return fixed target extension.
diff --git a/build2/target-key b/build2/target-key
index 7905f8a..0eae11f 100644
--- a/build2/target-key
+++ b/build2/target-key
@@ -26,9 +26,7 @@ namespace build2
     const dir_path* const dir; // Can be relative if part of prerequisite_key.
     const dir_path* const out; // Can be relative if part of prerequisite_key.
     const string* const name;
-    const optional<string>& ext;
-
-    static const optional<string> nullext;
+    mutable optional<string> ext; // Absent - unspecified, empty - none.
 
     template <typename T>
     bool is_a () const {return type->is_a<T> ();}
diff --git a/build2/target-type b/build2/target-type
index ab21aa0..81f6916 100644
--- a/build2/target-type
+++ b/build2/target-type
@@ -41,11 +41,13 @@ namespace build2
     const char* name;
     const target_type* base;
 
-    target* (*factory) (const target_type&,
-                        dir_path,
-                        dir_path,
-                        string,
-                        optional<string>);
+    // Return target and extension.
+    //
+    pair<target*, optional<string>> (*factory) (const target_type&,
+                                                dir_path,
+                                                dir_path,
+                                                string,
+                                                optional<string>);
 
     optional<string> (*extension) (const target_key&,
                                    const scope&,
diff --git a/build2/target.cxx b/build2/target.cxx
index 30e580a..ca3f6f2 100644
--- a/build2/target.cxx
+++ b/build2/target.cxx
@@ -17,10 +17,6 @@ using namespace std;
 
 namespace build2
 {
-  // target_key
-  //
-  const optional<string> target_key::nullext = nullopt;
-
   // target_type
   //
   bool target_type::
@@ -205,30 +201,29 @@ namespace build2
   auto target_set::
   find (const target_key& k, tracer& trace) const -> iterator
   {
-    iterator i (map_.find (k));
+    map::const_iterator i (map_.find (k));
 
-    if (i != end ())
+    if (i != map_.end ())
     {
-      target& t (**i);
+      target& t (*i->second);
+      optional<string>& ext (i->first.ext);
 
-      // Update the extension if the existing target has it unspecified.
-      //
-      const optional<string>& ext (k.ext);
-      if (t.ext != ext)
+      if (ext != k.ext)
       {
         l5 ([&]{
             diag_record r (trace);
             r << "assuming target " << t << " is the same as the one with ";
-            if (!ext)
+            if (!k.ext)
               r << "unspecified extension";
-            else if (ext->empty ())
+            else if (k.ext->empty ())
               r << "no extension";
             else
-              r << "extension " << *ext;
+              r << "extension " << *k.ext;
           });
 
-        if (ext)
-          t.ext = ext;
+
+        if (k.ext)
+          ext = k.ext;
       }
     }
 
@@ -244,19 +239,27 @@ namespace build2
           bool implied,
           tracer& trace)
   {
-    iterator i (find (target_key {&tt, &dir, &out, &name, ext}, trace));
+    target_key tk {&tt, &dir, &out, &name, move (ext)};
+
+    iterator i (find (tk, trace));
     bool r (i == end ());
 
     if (r)
     {
-      unique_ptr<target> pt (
-        tt.factory (tt, move (dir), move (out), move (name), move (ext)));
+      pair<target*, optional<string>> p (
+        tt.factory (
+          tt, move (dir), move (out), move (name), move (tk.ext)));
+
+      target& t (*p.first);
 
-      pt->implied = implied;
+      map::iterator j (
+        map_.emplace (
+          target_key {&tt, &t.dir, &t.out, &t.name, move (p.second)},
+          p.first).first);
 
-      i = map_.emplace (
-        make_pair (target_key {&tt, &pt->dir, &pt->out, &pt->name, pt->ext},
-                   move (pt))).first;
+      t.ext_ = &j->first.ext;
+      t.implied = implied;
+      i = j;
     }
     else if (!implied)
     {
@@ -357,6 +360,8 @@ namespace build2
     //
     assert (de == nullptr || type ().extension != nullptr);
 
+    optional<string>& ext (*ext_); //@@ MT lock
+
     if (!ext)
     {
       // If the target type has the extension function then try that first.
@@ -376,6 +381,9 @@ namespace build2
       }
     }
 
+    // Note that returning by reference is now MT-safe since once the
+    // extension is specified, it is immutable.
+    //
     return *ext;
   }
 
@@ -403,12 +411,14 @@ namespace build2
   {
     // Derive and add the extension if any.
     //
-    derive_extension (de);
-
-    if (!ext->empty ())
     {
-      p += '.';
-      p += *ext;
+      const string& e (derive_extension (de));
+
+      if (!e.empty ())
+      {
+        p += '.';
+        p += e;
+      }
     }
 
     const path_type& ep (path ());
@@ -521,7 +531,7 @@ namespace build2
   };
 
   template <typename T, const char* ext>
-  static target*
+  static pair<target*, optional<string>>
   file_factory (const target_type& tt,
                 dir_path d,
                 dir_path o,
@@ -534,13 +544,10 @@ namespace build2
     // it to fixed ext rather than unspecified. For file{} we make it empty
     // which means we treat file{foo} as file{foo.}.
     //
-    return new T (
-      move (d),
-      move (o),
-      move (n),
-      (e || ext == nullptr || tt.factory != &file_factory<T, ext>
-       ? move (e)
-       : string (ext)));
+    if (!e && ext != nullptr && tt.factory == &file_factory<T, ext>)
+      e = string (ext);
+
+    return make_pair (new T (move (d), move (o), move (n)), move (e));
   }
 
   extern const char file_ext_var[] = "extension"; // VC14 rejects constexpr.
@@ -718,7 +725,7 @@ namespace build2
     false
   };
 
-  static target*
+  static pair<target*, optional<string>>
   buildfile_factory (const target_type&,
                      dir_path d,
                      dir_path o,
@@ -728,7 +735,7 @@ namespace build2
     if (!e)
       e = (n == "buildfile" ? string () : "build");
 
-    return new buildfile (move (d), move (o), move (n), move (e));
+    return make_pair (new buildfile (move (d), move (o), move (n)), move (e));
   }
 
   static optional<string>
@@ -762,7 +769,7 @@ namespace build2
     false
   };
 
-  static target*
+  static pair<target*, optional<string>>
   man_factory (const target_type&,
                dir_path d,
                dir_path o,
@@ -772,7 +779,7 @@ namespace build2
     if (!e)
       fail << "man target '" << n << "' must include extension (man section)";
 
-    return new man (move (d), move (o), move (n), move (e));
+    return make_pair (new man (move (d), move (o), move (n)), move (e));
   }
 
   const target_type man::static_type
diff --git a/build2/test/target.cxx b/build2/test/target.cxx
index a5801e8..3bf00c1 100644
--- a/build2/test/target.cxx
+++ b/build2/test/target.cxx
@@ -11,7 +11,7 @@ namespace build2
 {
   namespace test
   {
-    static target*
+    static pair<target*, optional<string>>
     testscript_factory (const target_type&,
                         dir_path d,
                         dir_path o,
@@ -21,7 +21,8 @@ namespace build2
       if (!e)
         e = (n == "testscript" ? string () : "test");
 
-      return new testscript (move (d), move (o), move (n), move (e));
+      return make_pair (
+        new testscript (move (d), move (o), move (n)), move (e));
     }
 
     static optional<string>
-- 
cgit v1.1