From a89f1e4f4efd291beedea03c65c8185b7d0df20e Mon Sep 17 00:00:00 2001
From: Boris Kolpackov <boris@codesynthesis.com>
Date: Thu, 7 Dec 2017 12:06:59 +0200
Subject: Distinguish between "fixed" and "default" target extensions

This fixes wrong merging of, say, file{README} and file{README.MySQL}
(in libmysqlclient).
---
 build2/bin/target.cxx  | 100 ++++++++++++++++++-----------------
 build2/cc/compile.cxx  |   2 +-
 build2/cc/target.cxx   |  24 ++++++---
 build2/cli/target.cxx  |  19 ++++---
 build2/context.cxx     |   4 +-
 build2/context.hxx     |   2 +
 build2/cxx/target.cxx  |  29 +++++-----
 build2/scope.cxx       |  64 +++++++++++++---------
 build2/search.cxx      |   4 +-
 build2/target-key.hxx  |  26 ++++++---
 build2/target-type.hxx |  28 +++++-----
 build2/target.cxx      | 140 ++++++++++++++++++++++++-------------------------
 build2/target.hxx      |  36 +++----------
 build2/target.txx      |  20 ++++---
 build2/test/target.cxx |  23 ++------
 15 files changed, 270 insertions(+), 251 deletions(-)

diff --git a/build2/bin/target.cxx b/build2/bin/target.cxx
index d9dc64b..533da43 100644
--- a/build2/bin/target.cxx
+++ b/build2/bin/target.cxx
@@ -2,6 +2,8 @@
 // copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
 // license   : MIT; see accompanying LICENSE file
 
+#include <build2/context.hxx>
+
 #include <build2/bin/target.hxx>
 
 using namespace std;
@@ -18,6 +20,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -30,6 +33,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -41,24 +45,18 @@ namespace build2
     // running serial. For the members it is also safe to set the group during
     // creation.
 
-    extern const char ext_var[] = "extension"; // VC14 rejects constexpr.
-
     // obj*{}, bmi*{}, libu*{} member factory.
     //
     template <typename M, typename G>
-    static pair<target*, optional<string>>
-    m_factory (const target_type&,
-               dir_path dir,
-               dir_path out,
-               string n,
-               optional<string> ext)
+    static target*
+    m_factory (const target_type&, dir_path dir, dir_path out, string n)
     {
       const G* g (targets.find<G> (dir, out, n));
 
       M* m (new M (move (dir), move (out), move (n)));
       m->group = g;
 
-      return make_pair (m, move (ext));
+      return m;
     }
 
     const target_type obje::static_type
@@ -66,8 +64,9 @@ namespace build2
       "obje",
       &file::static_type,
       &m_factory<obje, obj>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -78,8 +77,9 @@ namespace build2
       "bmie",
       &file::static_type,
       &m_factory<bmie, bmi>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -90,8 +90,9 @@ namespace build2
       "libue",
       &libux::static_type,
       &m_factory<libue, libu>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -102,8 +103,9 @@ namespace build2
       "obja",
       &file::static_type,
       &m_factory<obja, obj>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -114,8 +116,9 @@ namespace build2
       "bmia",
       &file::static_type,
       &m_factory<bmia, bmi>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -126,8 +129,9 @@ namespace build2
       "libua",
       &libux::static_type,
       &m_factory<libua, libu>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -138,8 +142,9 @@ namespace build2
       "objs",
       &file::static_type,
       &m_factory<objs, obj>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -150,8 +155,9 @@ namespace build2
       "bmis",
       &file::static_type,
       &m_factory<bmis, bmi>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -162,8 +168,9 @@ namespace build2
       "libus",
       &libux::static_type,
       &m_factory<libus, libu>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &target_search, // Note: not _file(); don't look for an existing file.
       false
@@ -172,12 +179,8 @@ namespace build2
     // obj{}, bmi{}, and libu{} group factory.
     //
     template <typename G, typename E, typename A, typename S>
-    static pair<target*, optional<string>>
-    g_factory (const target_type&,
-               dir_path dir,
-               dir_path out,
-               string n,
-               optional<string> ext)
+    static target*
+    g_factory (const target_type&, dir_path dir, dir_path out, string n)
     {
       // Casts are MT-aware (during serial load).
       //
@@ -197,7 +200,7 @@ namespace build2
       if (a != nullptr) a->group = g;
       if (s != nullptr) s->group = g;
 
-      return make_pair (g, move (ext));
+      return g;
     }
 
     const target_type obj::static_type
@@ -208,6 +211,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -220,6 +224,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -232,6 +237,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -251,8 +257,9 @@ namespace build2
       "liba",
       &file::static_type,
       &m_factory<liba, lib>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &file_search,
       false
@@ -263,8 +270,9 @@ namespace build2
       "libs",
       &file::static_type,
       &m_factory<libs, lib>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &file_search,
       false
@@ -283,12 +291,8 @@ namespace build2
         : group_view {nullptr, 0};
     }
 
-    static pair<target*, optional<string>>
-    lib_factory (const target_type&,
-                 dir_path dir,
-                 dir_path out,
-                 string n,
-                 optional<string> ext)
+    static target*
+    lib_factory (const target_type&, dir_path dir, dir_path out, string n)
     {
       // Casts are MT-aware (during serial load).
       //
@@ -304,7 +308,7 @@ namespace build2
       if (a != nullptr) a->group = l;
       if (s != nullptr) s->group = l;
 
-      return make_pair (l, move (ext));
+      return l;
     }
 
     const target_type lib::static_type
@@ -315,6 +319,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -326,8 +331,9 @@ namespace build2
       "libi",
       &file::static_type,
       &target_factory<libi>,
-      &target_extension_var<ext_var, nullptr>,
-      &target_pattern_var<ext_var, nullptr>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, nullptr>,
+      &target_pattern_var<var_extension, nullptr>,
       nullptr,
       &file_search,
       false
diff --git a/build2/cc/compile.cxx b/build2/cc/compile.cxx
index ca07b42..a81f147 100644
--- a/build2/cc/compile.cxx
+++ b/build2/cc/compile.cxx
@@ -1012,7 +1012,7 @@ namespace build2
 
         // This is like prerequisite search.
         //
-        if (optional<string> de = tt.extension (tk, s, true))
+        if (optional<string> de = tt.default_extension (tk, s, true))
           if (*de == e)
             return true;
 
diff --git a/build2/cc/target.cxx b/build2/cc/target.cxx
index 6f8d541..38236a0 100644
--- a/build2/cc/target.cxx
+++ b/build2/cc/target.cxx
@@ -2,6 +2,8 @@
 // copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
 // license   : MIT; see accompanying LICENSE file
 
+#include <build2/context.hxx>
+
 #include <build2/cc/target.hxx>
 
 using namespace std;
@@ -18,33 +20,36 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
 
-    extern const char ext_var[] = "extension";  // VC14 rejects constexpr.
-
     extern const char h_ext_def[] = "h";
+
     const target_type h::static_type
     {
       "h",
       &file::static_type,
       &target_factory<h>,
-      &target_extension_var<ext_var, h_ext_def>,
-      &target_pattern_var<ext_var, h_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, h_ext_def>,
+      &target_pattern_var<var_extension, h_ext_def>,
       nullptr,
       &file_search,
       false
     };
 
     extern const char c_ext_def[] = "c";
+
     const target_type c::static_type
     {
       "c",
       &cc::static_type,
       &target_factory<c>,
-      &target_extension_var<ext_var, c_ext_def>,
-      &target_pattern_var<ext_var, c_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, c_ext_def>,
+      &target_pattern_var<var_extension, c_ext_def>,
       nullptr,
       &file_search,
       false
@@ -58,6 +63,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       false
     };
@@ -68,8 +74,9 @@ namespace build2
     {
       "pca",
       &pc::static_type,
-      &file_factory<pca, pca_ext>,
+      &target_factory<pca>,
       &target_extension_fix<pca_ext>,
+      nullptr, /* default_extension */
       &target_pattern_fix<pca_ext>,
       &target_print_0_ext_verb, // Fixed extension, no use printing.
       &file_search,
@@ -82,8 +89,9 @@ namespace build2
     {
       "pcs",
       &pc::static_type,
-      &file_factory<pcs, pcs_ext>,
+      &target_factory<pcs>,
       &target_extension_fix<pcs_ext>,
+      nullptr, /* default_extension */
       &target_pattern_fix<pcs_ext>,
       &target_print_0_ext_verb, // Fixed extension, no use printing.
       &file_search,
diff --git a/build2/cli/target.cxx b/build2/cli/target.cxx
index 10aab5e..c35ee18 100644
--- a/build2/cli/target.cxx
+++ b/build2/cli/target.cxx
@@ -2,6 +2,8 @@
 // copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
 // license   : MIT; see accompanying LICENSE file
 
+#include <build2/context.hxx>
+
 #include <build2/cli/target.hxx>
 
 using namespace std;
@@ -13,7 +15,6 @@ namespace build2
   {
     // cli
     //
-    extern const char cli_ext_var[] = "extension";  // VC14 rejects constexpr.
     extern const char cli_ext_def[] = "cli";
 
     const target_type cli::static_type
@@ -21,8 +22,9 @@ namespace build2
       "cli",
       &file::static_type,
       &target_factory<cli>,
-      &target_extension_var<cli_ext_var, cli_ext_def>,
-      &target_pattern_var<cli_ext_var, cli_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, cli_ext_def>,
+      &target_pattern_var<var_extension, cli_ext_def>,
       nullptr,
       &file_search,
       false
@@ -42,12 +44,8 @@ namespace build2
         : group_view {nullptr, 0};
     }
 
-    static pair<target*, optional<string>>
-    cli_cxx_factory (const target_type&,
-                     dir_path d,
-                     dir_path o,
-                     string n,
-                     optional<string> e)
+    static target*
+    cli_cxx_factory (const target_type&, dir_path d, dir_path o, string n)
     {
       tracer trace ("cli::cli_cxx_factory");
 
@@ -61,7 +59,7 @@ namespace build2
       targets.insert<cxx::cxx> (d, o, n, trace);
       targets.insert<cxx::ixx> (d, o, n, trace);
 
-      return make_pair (new cli_cxx (move (d), move (o), move (n)), move (e));
+      return new cli_cxx (move (d), move (o), move (n));
     }
 
     const target_type cli_cxx::static_type
@@ -72,6 +70,7 @@ namespace build2
       nullptr,
       nullptr,
       nullptr,
+      nullptr,
       &target_search,
       true // "See through" default iteration mode.
     };
diff --git a/build2/context.cxx b/build2/context.cxx
index b359d8f..b12920c 100644
--- a/build2/context.cxx
+++ b/build2/context.cxx
@@ -339,6 +339,8 @@ namespace build2
 
   const variable* var_clean;
 
+  const char var_extension[10] = "extension";
+
   const string* current_mname;
   const string* current_oname;
 
@@ -413,7 +415,7 @@ namespace build2
 
     // Target extension.
     //
-    vp.insert<string> ("extension", variable_visibility::target);
+    vp.insert<string> (var_extension, variable_visibility::target);
 
     gs.assign<dir_path> ("build.work") = work;
     gs.assign<dir_path> ("build.home") = home;
diff --git a/build2/context.hxx b/build2/context.hxx
index 434b41f..fed6bf1 100644
--- a/build2/context.hxx
+++ b/build2/context.hxx
@@ -275,6 +275,8 @@ namespace build2
 
   extern const variable* var_clean;           // [bool] target visibility
 
+  extern const char var_extension[10];        // "extension"
+
   // Current action (meta/operation).
   //
   // The names unlike info are available during boot but may not yet be
diff --git a/build2/cxx/target.cxx b/build2/cxx/target.cxx
index bde41fc..b7c3f8c 100644
--- a/build2/cxx/target.cxx
+++ b/build2/cxx/target.cxx
@@ -2,6 +2,8 @@
 // copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
 // license   : MIT; see accompanying LICENSE file
 
+#include <build2/context.hxx>
+
 #include <build2/cxx/target.hxx>
 
 using namespace std;
@@ -10,16 +12,15 @@ namespace build2
 {
   namespace cxx
   {
-    extern const char ext_var[] = "extension";  // VC14 rejects constexpr.
-
     extern const char hxx_ext_def[] = "hxx";
     const target_type hxx::static_type
     {
       "hxx",
       &file::static_type,
       &target_factory<hxx>,
-      &target_extension_var<ext_var, hxx_ext_def>,
-      &target_pattern_var<ext_var, hxx_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, hxx_ext_def>,
+      &target_pattern_var<var_extension, hxx_ext_def>,
       nullptr,
       &file_search,
       false
@@ -31,8 +32,9 @@ namespace build2
       "ixx",
       &file::static_type,
       &target_factory<ixx>,
-      &target_extension_var<ext_var, ixx_ext_def>,
-      &target_pattern_var<ext_var, ixx_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, ixx_ext_def>,
+      &target_pattern_var<var_extension, ixx_ext_def>,
       nullptr,
       &file_search,
       false
@@ -44,8 +46,9 @@ namespace build2
       "txx",
       &file::static_type,
       &target_factory<txx>,
-      &target_extension_var<ext_var, txx_ext_def>,
-      &target_pattern_var<ext_var, txx_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, txx_ext_def>,
+      &target_pattern_var<var_extension, txx_ext_def>,
       nullptr,
       &file_search,
       false
@@ -57,8 +60,9 @@ namespace build2
       "cxx",
       &cc::static_type,
       &target_factory<cxx>,
-      &target_extension_var<ext_var, cxx_ext_def>,
-      &target_pattern_var<ext_var, cxx_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, cxx_ext_def>,
+      &target_pattern_var<var_extension, cxx_ext_def>,
       nullptr,
       &file_search,
       false
@@ -70,8 +74,9 @@ namespace build2
       "mxx",
       &file::static_type,
       &target_factory<mxx>,
-      &target_extension_var<ext_var, mxx_ext_def>,
-      &target_pattern_var<ext_var, mxx_ext_def>,
+      nullptr, /* fixed_extension */
+      &target_extension_var<var_extension, mxx_ext_def>,
+      &target_pattern_var<var_extension, mxx_ext_def>,
       nullptr,
       &file_search,
       false
diff --git a/build2/scope.cxx b/build2/scope.cxx
index 6b6b18a..f10215e 100644
--- a/build2/scope.cxx
+++ b/build2/scope.cxx
@@ -639,12 +639,8 @@ namespace build2
     return r;
   }
 
-  static pair<target*, optional<string>>
-  derived_tt_factory (const target_type& t,
-                      dir_path d,
-                      dir_path o,
-                      string n,
-                      optional<string> e)
+  static target*
+  derived_tt_factory (const target_type& t, dir_path d, dir_path o, string n)
   {
     // Pass our type to the base factory so that it can detect that it is
     // being called to construct a derived target. This can be used, for
@@ -657,44 +653,64 @@ namespace build2
     const target_type* bt (t.base);
     for (; bt->factory == &derived_tt_factory; bt = bt->base) ;
 
-    auto r (bt->factory (t, move (d), move (o), move (n), move (e)));
-    r.first->derived_type = &t;
+    target* r (bt->factory (t, move (d), move (o), move (n)));
+    r->derived_type = &t;
     return r;
   }
 
-  // VC14 rejects constexpr.
-  //
-  extern const char derived_tt_ext_var[] = "extension";
-
   pair<reference_wrapper<const target_type>, bool> scope::
   derive_target_type (const string& name, const target_type& base)
   {
+    // Base target type uses extensions.
+    //
+    bool ext (base.fixed_extension   != nullptr ||
+              base.default_extension != nullptr);
+
     // @@ Looks like we may need the ability to specify a fixed extension
     //    (which will be used to compare existing targets and not just
     //    search for existing files that is handled by the target_type::
     //    extension hook). See the file_factory() for details. We will
     //    probably need to specify it as part of the define directive (and
-    //    have the ability to specify empty).
+    //    have the ability to specify empty and NULL).
     //
     //    Currently, if we define myfile{}: file{}, then myfile{foo} and
     //    myfile{foo.x} are the same target.
     //
-    // @@ Also, if derived from file{}, then we use its print function
-    //    which always prints extension by default (e.g., we get
-    //    dll{libhello.dll}).
-    //
-
     unique_ptr<target_type> dt (new target_type (base));
     dt->base = &base;
     dt->factory = &derived_tt_factory;
 
-    // Override extension derivation function: we most likely don't want
-    // to use the same default as our base (think cli: file). But, if our
-    // base doesn't use extensions, then most likely neither do we (think
-    // foo: alias).
+    // @@ We should probably inherit the fixed extension unless overriden with
+    // another fixed? But then any derivation from file{} will have to specify
+    // (or override) the fixed extension? But what is the use of deriving from
+    // a fixed extension target and not overriding its extension? Some kind of
+    // alias. Fuzzy.
+    //
+    dt->fixed_extension = nullptr /*&target_extension_fix<???>*/; // @@ TODO
+
+    // Override default extension/pattern derivation function: we most likely
+    // don't want to use the same default as our base (think cli: file). But,
+    // if our base doesn't use extensions, then most likely neither do we
+    // (think foo: alias).
+    //
+    dt->default_extension =
+      ext && dt->fixed_extension == nullptr
+      ? &target_extension_var<var_extension, nullptr>
+      : nullptr;
+
+    dt->pattern =
+      dt->fixed_extension != nullptr ? nullptr /*&target_pattern_fix<???>*/ :
+      dt->default_extension != nullptr ? &target_pattern_var<var_extension, nullptr> :
+      nullptr;
+
+    // There is actually a difference between "fixed fixed" (like man1{}) and
+    // "fixed but overridable" (like file{}). Fuzzy: feels like there are
+    // different kinds of "fixed" (file{} vs man{} vs man1{}).
     //
-    if (base.extension != nullptr)
-      dt->extension = &target_extension_var<derived_tt_ext_var, nullptr>;
+    dt->print =
+      dt->fixed_extension != nullptr
+      ? &target_print_0_ext_verb  // Fixed extension, no use printing.
+      : nullptr;                  // Normal.
 
     target_type& rdt (*dt); // Save a non-const reference to the object.
 
diff --git a/build2/search.cxx b/build2/search.cxx
index a7d40c6..f0b163a 100644
--- a/build2/search.cxx
+++ b/build2/search.cxx
@@ -102,7 +102,9 @@ namespace build2
 
     if (!ext)
     {
-      if (auto f = ctk.type->extension)
+      if (auto f = ctk.type->fixed_extension)
+        ext = f (ctk);
+      else if (auto f = ctk.type->default_extension)
         ext = f (ctk, *s, true);
 
       if (!ext)
diff --git a/build2/target-key.hxx b/build2/target-key.hxx
index b7335eb..f37713e 100644
--- a/build2/target-key.hxx
+++ b/build2/target-key.hxx
@@ -6,6 +6,7 @@
 #define BUILD2_TARGET_KEY_HXX
 
 #include <map>
+#include <cstring> // strcmp()
 
 #include <libbutl/utility.mxx> // compare_c_string
 
@@ -46,14 +47,25 @@ namespace build2
   inline bool
   operator== (const target_key& x, const target_key& y)
   {
-    // Unspecified and specified extension are assumed equal.
+    if (x.type  != y.type ||
+        *x.dir  != *y.dir ||
+        *x.out  != *y.out ||
+        *x.name != *y.name)
+      return false;
+
+    // Unless fixed, unspecified and specified extensions are assumed equal.
     //
-    return
-      x.type == y.type &&
-      *x.dir == *y.dir &&
-      *x.out == *y.out &&
-      *x.name == *y.name &&
-      (!x.ext || !y.ext || *x.ext == *y.ext);
+    const target_type& tt (*x.type);
+
+    if (tt.fixed_extension == nullptr)
+      return !x.ext || !y.ext || *x.ext == *y.ext;
+    else
+    {
+      const char* xe (x.ext ? x.ext->c_str () : tt.fixed_extension (x));
+      const char* ye (y.ext ? y.ext->c_str () : tt.fixed_extension (y));
+
+      return strcmp (xe, ye) == 0;
+    }
   }
 
   inline bool
diff --git a/build2/target-type.hxx b/build2/target-type.hxx
index 0b8ee54..16513d4 100644
--- a/build2/target-type.hxx
+++ b/build2/target-type.hxx
@@ -23,13 +23,16 @@ namespace build2
   // any target type. As a result, we can use address comparison to determine
   // if two target types are the same.
   //
-  // If the extension derivation function is NULL, then it means this target
+  // If the extension derivation functions are NULL, then it means this target
   // type does not use extensions. Note that this is relied upon when deciding
   // whether to print the extension; if the target does use extensions but the
-  // defaults could not (and its ok), could not (and its not ok), or should not
+  // defaults couldn't (and its ok), couldn't (and its not ok), or shouldn't
   // (logically) be obtained, then use target_extension_{null,fail,assert}(),
-  // respectively. If the extension function returns NULL, then that means the
-  // default extension for this target could not be derived.
+  // respectively. The fixed extension function should return the fixed
+  // extension (which can point to the key's ext member if the explicit
+  // extension specificaton is allowed). If the default extension function
+  // returns NULL, then it means the default extension for this target could
+  // not be derived.
   //
   // The extension is used in two places: search_existing_file() (called for a
   // prerequisite with the last argument true) and in target::derive_path()
@@ -45,17 +48,12 @@ namespace build2
     const char* name;
     const target_type* base;
 
-    // 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&,
-                                   bool search);
+    target* (*factory) (const target_type&, dir_path, dir_path, string);
+
+    const char*      (*fixed_extension)   (const target_key&);
+    optional<string> (*default_extension) (const target_key&,
+                                           const scope&,
+                                           bool search);
 
     bool (*pattern) (const target_type&, const scope&, string&, bool reverse);
 
diff --git a/build2/target.cxx b/build2/target.cxx
index 0dab7e2..aca977e 100644
--- a/build2/target.cxx
+++ b/build2/target.cxx
@@ -300,16 +300,14 @@ namespace build2
           ul.unlock ();
           return find (k, trace);
         }
-
-        ext = k.ext;
       }
 
       l5 ([&]{
           diag_record r (trace);
           r << "assuming target ";
           to_stream (r.os,
-                     target_key {&t.type (), &t.dir, &t.out, &t.name, nullopt},
-                     0); // Don't print the extension.
+                     target_key {&t.type (), &t.dir, &t.out, &t.name, ext},
+                     2); // Always print the extension.
           r << " is the same as the one with ";
 
           if (!k.ext)
@@ -319,6 +317,9 @@ namespace build2
           else
             r << "extension " << *k.ext;
         });
+
+      if (k.ext)
+        ext = k.ext;
     }
 
     return &t;
@@ -343,11 +344,11 @@ namespace build2
       //
       assert (phase != run_phase::execute);
 
-      pair<target*, optional<string>> te (
-        tt.factory (
-          tt, move (dir), move (out), move (name), move (tk.ext)));
+      optional<string> e (tt.fixed_extension != nullptr
+                          ? string (tt.fixed_extension (tk))
+                          : move (tk.ext));
 
-      t = te.first;
+      t = tt.factory (tt, move (dir), move (out), move (name));
 
       // Re-lock for exclusive access. In the meantime, someone could have
       // inserted this target so emplace() below could return false, in which
@@ -356,10 +357,8 @@ namespace build2
       //
       ulock ul (mutex_);
 
-      auto p (
-        map_.emplace (
-          target_key {&tt, &t->dir, &t->out, &t->name, te.second},
-          unique_ptr<target> (te.first)));
+      auto p (map_.emplace (target_key {&tt, &t->dir, &t->out, &t->name, e},
+                            unique_ptr<target> (t)));
 
       map_type::iterator i (p.first);
 
@@ -375,27 +374,27 @@ namespace build2
       t = i->second.get ();
       optional<string>& ext (i->first.ext);
 
-      if (ext != te.second)
+      if (ext != e)
       {
-        if (te.second)
-          ext = te.second;
-
         l5 ([&]{
             diag_record r (trace);
             r << "assuming target ";
             to_stream (
               r.os,
-              target_key {&t->type (), &t->dir, &t->out, &t->name, nullopt},
-              0); // Don't print the extension.
+              target_key {&t->type (), &t->dir, &t->out, &t->name, ext},
+              2); // Always print the extension.
             r << " is the same as the one with ";
 
-            if (!te.second)
+            if (!e)
               r << "unspecified extension";
-            else if (te.second->empty ())
+            else if (e->empty ())
               r << "no extension";
             else
-              r << "extension " << *te.second;
+              r << "extension " << *e;
           });
+
+        if (e)
+          ext = e;
       }
 
       // Fall through (continue as if the first find() returned this target).
@@ -434,16 +433,18 @@ namespace build2
         os << *k.dir;
     }
 
-    os << k.type->name << '{';
+    const target_type& tt (*k.type);
+
+    os << tt.name << '{';
 
     if (n)
     {
       os << *k.name;
 
-      // If the extension derivation function is NULL, then it means this
+      // If the extension derivation functions are NULL, then it means this
       // target type doesn't use extensions.
       //
-      if (k.type->extension != nullptr)
+      if (tt.fixed_extension != nullptr || tt.default_extension != nullptr)
       {
         // For verbosity level 0 we don't print the extension. For 1 we print
         // it if there is one. For 2 we print 'foo.?' if it hasn't yet been
@@ -542,8 +543,10 @@ namespace build2
   derive_extension (bool search, const char* de)
   {
     // See also search_existing_file() if updating anything here.
+
+    // The target should use extensions and they should not be fixed.
     //
-    assert (de == nullptr || type ().extension != nullptr);
+    assert (de == nullptr || type ().default_extension != nullptr);
 
     if (const string* p = ext ())
       // Note that returning by reference is now MT-safe since once the
@@ -554,12 +557,12 @@ namespace build2
     {
       optional<string> e;
 
-      // If the target type has the extension function then try that first.
-      // The reason for preferring it over what's been provided by the caller
-      // is that this function will often use the 'extension' variable which
-      // the user can use to override extensions.
+      // If the target type has the default extension function then try that
+      // first. The reason for preferring it over what's been provided by the
+      // caller is that this function will often use the 'extension' variable
+      // which the user can use to override extensions.
       //
-      if (auto f = type ().extension)
+      if (auto f = type ().default_extension)
         e = f (key (), base_scope (), search);
 
       if (!e)
@@ -680,6 +683,7 @@ namespace build2
     nullptr,
     nullptr,
     nullptr,
+    nullptr,
     &target_search,
     false
   };
@@ -692,6 +696,7 @@ namespace build2
     nullptr,
     nullptr,
     nullptr,
+    nullptr,
     &target_search,
     false
   };
@@ -704,20 +709,21 @@ namespace build2
     nullptr,
     nullptr,
     nullptr,
+    nullptr,
     &target_search,
     false
   };
 
-  extern const char file_ext_var[] = "extension"; // VC14 rejects constexpr.
   extern const char file_ext_def[] = "";
 
   const target_type file::static_type
   {
     "file",
     &path_target::static_type,
-    &file_factory<file, file_ext_def>,
-    &target_extension_var<file_ext_var, file_ext_def>,
-    &target_pattern_var<file_ext_var, file_ext_def>,
+    &target_factory<file>,
+    &target_extension_fix<file_ext_def>,
+    nullptr, /* default_extension */
+    nullptr, /* pattern */
     &target_print_1_ext_verb, // Print extension even at verbosity level 0.
     &file_search,
     false
@@ -745,6 +751,7 @@ namespace build2
     nullptr, // Extension not used.
     nullptr,
     nullptr,
+    nullptr,
     &alias_search,
     false
   };
@@ -886,6 +893,7 @@ namespace build2
     &alias::static_type,
     &target_factory<dir>,
     nullptr,              // Extension not used.
+    nullptr,
     &dir_pattern,
     nullptr,
     &dir_search,
@@ -898,6 +906,7 @@ namespace build2
     &target::static_type,
     &target_factory<fsdir>,
     nullptr,              // Extension not used.
+    nullptr,
     &dir_pattern,
     nullptr,
     &target_search,
@@ -948,6 +957,7 @@ namespace build2
     "exe",
     &file::static_type,
     &target_factory<exe>,
+    nullptr, /* fixed_extension */
     &exe_target_extension,
 #ifdef _WIN32
     &exe_target_pattern,
@@ -959,26 +969,13 @@ namespace build2
     false
   };
 
-  static pair<target*, optional<string>>
-  buildfile_factory (const target_type&,
-                     dir_path d,
-                     dir_path o,
-                     string n,
-                     optional<string> e)
-  {
-    if (!e)
-      e = (n == "buildfile" ? string () : "build");
-
-    return make_pair (new buildfile (move (d), move (o), move (n)), move (e));
-  }
-
-  static optional<string>
-  buildfile_target_extension (const target_key& tk, const scope&, bool)
+  static const char*
+  buildfile_target_extension (const target_key& tk)
   {
     // If the name is special 'buildfile', then there is no extension,
     // otherwise it is .build.
     //
-    return *tk.name == "buildfile" ? string () : "build";
+    return *tk.name == "buildfile" ? "" : "build";
   }
 
   static bool
@@ -1007,8 +1004,9 @@ namespace build2
   {
     "build",
     &file::static_type,
-    &buildfile_factory,
+    &target_factory<buildfile>,
     &buildfile_target_extension,
+    nullptr, /* default_extension */
     &buildfile_target_pattern,
     nullptr,
     &file_search,
@@ -1020,7 +1018,7 @@ namespace build2
   static const target*
   in_search (const target& xt, const prerequisite_key& cpk)
   {
-    // If we have no extension then derive it from our target. The delegate
+    // If we have no extension then derive it from our target. Then delegate
     // to file_search().
     //
     prerequisite_key pk (cpk);
@@ -1050,10 +1048,11 @@ namespace build2
   {
     "in",
     &file::static_type,
-    &file_factory<in, file_ext_def>, // No extension by default.
-    &target_extension_assert,        // Should be taken care by search.
+    &target_factory<in>,
+    &target_extension_fix<file_ext_def>, // No extension by default.
+    nullptr, /* default_extension */     // Should be taken care if by search.
     &in_pattern,
-    &target_print_1_ext_verb,        // Same as file.
+    &target_print_1_ext_verb,            // Same as file.
     &in_search,
     false
   };
@@ -1062,33 +1061,31 @@ namespace build2
   {
     "doc",
     &file::static_type,
-    &file_factory<doc, file_ext_def>, // No extension by default.
-    &target_extension_var<file_ext_var, file_ext_def>, // Same as file.
-    &target_pattern_var<file_ext_var, file_ext_def>,   // Same as file.
-    &target_print_1_ext_verb, // Same as file.
+    &target_factory<doc>,
+    &target_extension_fix<file_ext_def>, // Same as file (no extension).
+    nullptr, /* default_extension */
+    nullptr, /* pattern */               // Same as file.
+    &target_print_1_ext_verb,            // Same as file.
     &file_search,
     false
   };
 
-  static pair<target*, optional<string>>
-  man_factory (const target_type&,
-               dir_path d,
-               dir_path o,
-               string n,
-               optional<string> e)
+  static const char*
+  man_extension (const target_key& tk)
   {
-    if (!e)
-      fail << "man target '" << n << "' must include extension (man section)";
+    if (!tk.ext)
+      fail << "man target " << tk << " must include extension (man section)";
 
-    return make_pair (new man (move (d), move (o), move (n)), move (e));
+    return tk.ext->c_str ();
   }
 
   const target_type man::static_type
   {
     "man",
     &doc::static_type,
-    &man_factory,
-    &target_extension_null, // Should be specified explicitly (see factory).
+    &target_factory<man>,
+    &man_extension,           // Should be specified explicitly.
+    nullptr, /* default_extension */
     nullptr,
     &target_print_1_ext_verb, // Print extension even at verbosity level 0.
     &file_search,
@@ -1101,8 +1098,9 @@ namespace build2
   {
     "man1",
     &man::static_type,
-    &file_factory<man1, man1_ext>,
+    &target_factory<man1>,
     &target_extension_fix<man1_ext>,
+    nullptr,  /* default_extension */
     &target_pattern_fix<man1_ext>,
     &target_print_0_ext_verb, // Fixed extension, no use printing.
     &file_search,
diff --git a/build2/target.hxx b/build2/target.hxx
index 73ee2d8..10b0eb3 100644
--- a/build2/target.hxx
+++ b/build2/target.hxx
@@ -1672,41 +1672,17 @@ namespace build2
   // functions.
   //
   template <typename T>
-  pair<target*, optional<string>>
-  target_factory (const target_type&,
-                  dir_path d,
-                  dir_path o,
-                  string n,
-                  optional<string> e)
+  target*
+  target_factory (const target_type&, dir_path d, dir_path o, string n)
   {
-    return make_pair (new T (move (d), move (o), move (n)), move (e));
+    return new T (move (d), move (o), move (n));
   }
 
-  template <typename T, const char* ext>
-  pair<target*, optional<string>>
-  file_factory (const target_type& tt,
-                dir_path d,
-                dir_path o,
-                string n,
-                optional<string> e)
-  {
-    // A generic file target type doesn't imply any extension while a very
-    // specific one (say man1) may have a fixed extension. So if one wasn't
-    // specified and this is not a dynamically derived target type, then set
-    // it to fixed ext rather than unspecified. For file{} itself we make it
-    // empty which means we treat file{foo} as file{foo.}.
-    //
-    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));
-  }
-
-  // Return fixed target extension.
+  // Return fixed target extension unless one was specified.
   //
   template <const char* ext>
-  optional<string>
-  target_extension_fix (const target_key&, const scope&, bool);
+  const char*
+  target_extension_fix (const target_key&);
 
   template <const char* ext>
   bool
diff --git a/build2/target.txx b/build2/target.txx
index a91166c..5cb4732 100644
--- a/build2/target.txx
+++ b/build2/target.txx
@@ -42,10 +42,15 @@ namespace build2
   //
   //
   template <const char* ext>
-  optional<string>
-  target_extension_fix (const target_key&, const scope&, bool)
+  const char*
+  target_extension_fix (const target_key& tk)
   {
-    return string (ext);
+    // A generic file target type doesn't imply any extension while a very
+    // specific one (say man1) may have a fixed extension. So if one wasn't
+    // specified set it to fixed ext rather than unspecified. For file{}
+    // itself we make it empty which means we treat file{foo} as file{foo.}.
+    //
+    return tk.ext ? tk.ext->c_str () : ext;
   }
 
   template <const char* ext>
@@ -67,9 +72,12 @@ namespace build2
     //
     else if (p == string::npos)
     {
-      v += '.';
-      v += ext;
-      return true;
+      if (*ext != '\0') // Don't add empty extension (means no extension).
+      {
+        v += '.';
+        v += ext;
+        return true;
+      }
     }
 
     return false;
diff --git a/build2/test/target.cxx b/build2/test/target.cxx
index a85546c..0bda827 100644
--- a/build2/test/target.cxx
+++ b/build2/test/target.cxx
@@ -11,27 +11,13 @@ namespace build2
 {
   namespace test
   {
-    static pair<target*, optional<string>>
-    testscript_factory (const target_type&,
-                        dir_path d,
-                        dir_path o,
-                        string n,
-                        optional<string> e)
-    {
-      if (!e)
-        e = (n == "testscript" ? string () : "test");
-
-      return make_pair (
-        new testscript (move (d), move (o), move (n)), move (e));
-    }
-
-    static optional<string>
-    testscript_target_extension (const target_key& tk, const scope&, bool)
+    static const char*
+    testscript_target_extension (const target_key& tk)
     {
       // If the name is special 'testscript', then there is no extension,
       // otherwise it is .test.
       //
-      return *tk.name == "testscript" ? string () : "test";
+      return *tk.name == "testscript" ? "" : "test";
     }
 
     static bool
@@ -60,8 +46,9 @@ namespace build2
     {
       "test",
       &file::static_type,
-      &testscript_factory,
+      &target_factory<testscript>,
       &testscript_target_extension,
+      nullptr,  /* default_extension */
       &testscript_target_pattern,
       nullptr,
       &file_search,
-- 
cgit v1.1