Rework default extension derivation, again

7 files changed, 90 insertions, 94 deletions

diff --git a/build2/bin/target.cxx b/build2/bin/target.cxx
index de51fc7..ed36f80 100644
--- a/build2/bin/target.cxx
+++ b/build2/bin/target.cxx
@@ -10,6 +10,8 @@ namespace build2
 {
   namespace bin
   {
+    constexpr const char ext_var[] = "extension";
+
     static target*
     obja_factory (const target_type&, dir_path d, string n, const string* e)
     {
@@ -27,7 +29,7 @@ namespace build2
       "obja",
       &file::static_type,
       &obja_factory,
-      &target_extension_assert,
+      &target_extension_var<ext_var, nullptr>,
       nullptr,
       &search_target, // Note: not _file(); don't look for an existing file.
       false
@@ -50,7 +52,7 @@ namespace build2
       "objso",
       &file::static_type,
       &objso_factory,
-      &target_extension_assert,
+      &target_extension_var<ext_var, nullptr>,
       nullptr,
       &search_target, // Note: not _file(); don't look for an existing file.
       false
@@ -94,13 +96,12 @@ namespace build2
     //    (or any module that knows how to build exe{}) changes it to the
     //    'host'. Maybe that's not a bad idea?
     //
-    constexpr const char exe_ext[] = "";
     const target_type exe::static_type
     {
       "exe",
       &file::static_type,
       &target_factory<exe>,
-      &target_extension_fix<exe_ext>,
+      &target_extension_var<ext_var, nullptr>,
       nullptr,
       &search_file,
       false
@@ -132,13 +133,12 @@ namespace build2
     // come from a variable that is set not by bin but by the module
     // whose rule matched the target (e.g., cxx::link).
     //
-    constexpr const char a_ext[] = "a";
     const target_type liba::static_type
     {
       "liba",
       &file::static_type,
       &liba_factory,
-      &target_extension_fix<a_ext>,
+      &target_extension_var<ext_var, nullptr>,
       nullptr,
       &search_file,
       false
@@ -158,13 +158,12 @@ namespace build2
       return so;
     }
 
-    constexpr const char so_ext[] = "so";
     const target_type libso::static_type
     {
       "libso",
       &file::static_type,
       &libso_factory,
-      &target_extension_fix<so_ext>,
+      &target_extension_var<ext_var, nullptr>,
       nullptr,
       &search_file,
       false
diff --git a/build2/cxx/link.cxx b/build2/cxx/link.cxx
index 7d17553..2f9161c 100644
--- a/build2/cxx/link.cxx
+++ b/build2/cxx/link.cxx
@@ -238,15 +238,13 @@ namespace build2
 
       if (l || p.is_a<liba> ())
       {
-        an = path ("lib" + p.name);
-
-        // Note that p.scope should be the same as the target's for
-        // which we are looking for this library. The idea here is
-        // that we have to use the same "extension configuration" as
-        // the target's.
+        // We are trying to find a library in the search paths extracted from
+        // the compiler. It would only be natural if we use the library
+        // prefix/extension that correspond to this compiler's target.
         //
+        an = path ("lib" + p.name);
         ae = ext == nullptr
-          ? &liba::static_type.extension (p.key ().tk, p.scope)
+          ? &extension_pool.find ("a")
           : ext;
 
         if (!ae->empty ())
@@ -265,7 +263,7 @@ namespace build2
       {
         sn = path ("lib" + p.name);
         se = ext == nullptr
-          ? &libso::static_type.extension (p.key ().tk, p.scope)
+          ? &extension_pool.find ("so")
           : ext;
 
         if (!se->empty ())
@@ -487,22 +485,21 @@ namespace build2
       //
       if (t.path ().empty ())
       {
-        auto l (t["extension"]);
-        const char* e (l ? as<string> (*l).c_str () : nullptr);
-
         switch (lt)
         {
         case type::e:
           {
-            t.derive_path (e != nullptr ? e : "");
+            t.derive_path ("");
             break;
           }
         case type::a:
         case type::so:
           {
             auto l (t["bin.libprefix"]);
-            t.derive_path (e != nullptr ? e : (lt == type::a ? "a" : "so"),
-                           l ? as<string> (*l).c_str () : "lib");
+            const char* e (lt == type::a ? "a" : "so");
+            const char* p (l ? as<string> (*l).c_str () : "lib");
+
+            t.derive_path (e, p);
             break;
           }
         }
diff --git a/build2/search.cxx b/build2/search.cxx
index 2ab3d1f..3df8877 100644
--- a/build2/search.cxx
+++ b/build2/search.cxx
@@ -71,20 +71,18 @@ namespace build2
     {
       if (auto f = ctk.type->extension)
       {
-        ext = &f (ctk, *cpk.scope); // Already from the pool.
+        ext = f (ctk, *cpk.scope); // Already from the pool.
       }
-      else
+
+      if (ext == nullptr)
       {
         // What should we do here, fail or say we didn't find anything?
-        // Current think is that if the target type didn't provide the
-        // default extension, then it doesn't want us to search for an
-        // existing file (of course, if the user specified the extension
-        // explicitly, we will still do so). But let me know what you
-        // think.
+        // Current think is that if the target type couldn't find the default
+        // extension, then we simply shouldn't search for any existing files
+        // (of course, if the user specified the extension explicitly, we will
+        // still do so).
         //
-        //fail << "no default extension for prerequisite " << cpk;
-        level4 ([&]{trace << "no existing file found for prerequisite "
-                          << cpk;});
+        level4 ([&]{trace << "no existing file for prerequisite " << cpk;});
         return nullptr;
       }
     }
@@ -134,7 +132,7 @@ namespace build2
       return &t;
     }
 
-    level4 ([&]{trace << "no existing file found for prerequisite " << cpk;});
+    level4 ([&]{trace << "no existing file for prerequisite " << cpk;});
     return nullptr;
   }
 
diff --git a/build2/target b/build2/target
index e6ebc42..b35efdf 100644
--- a/build2/target
+++ b/build2/target
@@ -879,18 +879,19 @@ namespace build2
     void
     path (path_type p) {assert (path_.empty ()); path_ = std::move (p);}
 
-    // Derive a path from target's dir, name, and, if specified, ext.
-    // If ext is not specified, then use default_ext and also update
-    // the target's extension (this becomes important if later we need
-    // to reliably determine whether this file has an extension; think
-    // hxx{foo.bar.} and hxx.ext is empty).
+    // Derive a path from target's dir, name, and, if set, ext. If ext is not
+    // set, try to derive it using the target type extension function and
+    // fallback to default_ext, if specified. In both cases also update the
+    // target's extension (this becomes important if later we need to reliably
+    // determine whether this file has an extension; think hxx{foo.bar.} and
+    // hxx{*}:extension is empty).
     //
-    // If name_prefix is not NULL, add it before the name part and after
-    // the directory. Similarly, if name_suffix is not NULL, add it after
-    // the name part and before the extension.
+    // If name_prefix is not NULL, add it before the name part and after the
+    // directory. Similarly, if name_suffix is not NULL, add it after the name
+    // part and before the extension.
     //
-    // Finally, if the path was already assigned to this target, then
-    // this function verifies that the two are the same.
+    // Finally, if the path was already assigned to this target, then this
+    // function verifies that the two are the same.
     //
     void
     derive_path (const char* default_ext = nullptr,
@@ -1058,24 +1059,29 @@ namespace build2
   // Return fixed target extension.
   //
   template <const char* ext>
-  const std::string&
+  const std::string*
   target_extension_fix (const target_key&, scope&);
 
-  // Get the extension from the variable or use the default if none set.
-  // Issue diagnostics and fail if the default is NULL.
+  // Get the extension from the variable or use the default if none set. If
+  // the default is NULL, then return NULL.
   //
   template <const char* var, const char* def>
-  const std::string&
+  const std::string*
   target_extension_var (const target_key&, scope&);
 
+  // Always return NULL extension.
+  //
+  const std::string*
+  target_extension_null (const target_key&, scope&);
+
   // Issue diagnostics and fail if called.
   //
-  const std::string&
+  const std::string*
   target_extension_fail (const target_key&, scope&);
 
   // Assert if called.
   //
-  const std::string&
+  const std::string*
   target_extension_assert (const target_key&, scope&);
 
   // Target print functions.
diff --git a/build2/target-type b/build2/target-type
index a831670..2659e3d 100644
--- a/build2/target-type
+++ b/build2/target-type
@@ -28,15 +28,20 @@ namespace build2
   // If the extension derivation function is 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 should not or cannot (logically) be obtained, then use
-  // target_extension_{fail,assert}(), respectively.
+  // defaults could not (and its ok), could not (and its not ok), or should not
+  // (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.
+  //
+  // The extension function is used in two places: search_existing_file() and
+  // in target::derive_path(); see their implementations for details.
   //
   struct target_type
   {
     const char* name;
     const target_type* base;
     target* (*factory) (const target_type&, dir_path, string, const string*);
-    const string& (*extension) (const target_key&, scope&);
+    const string* (*extension) (const target_key&, scope&);
     void (*print) (ostream&, const target_key&);
     target* (*search) (const prerequisite_key&);
     bool see_through; // A group with the default "see through" semantics.
diff --git a/build2/target.cxx b/build2/target.cxx
index 3fd099d..50188ac 100644
--- a/build2/target.cxx
+++ b/build2/target.cxx
@@ -322,24 +322,28 @@ namespace build2
     if (ns != nullptr)
       n += ns;
 
-    // Update the extension.
-    //
-    // See also search_existing_file() if updating anything here.
+    // Update the extension. See also search_existing_file() if updating
+    // anything here.
     //
+    assert (de == nullptr || type ().extension != nullptr);
+
     if (ext == nullptr)
     {
-      // If provided by the caller, then use that.
-      //
-      if (de != nullptr)
-        ext = &extension_pool.find (de);
+      // 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.
       //
-      // Otherwise see if the target type has function that will
-      // give us the default extension.
-      //
-      else if (auto f = type ().extension)
-        ext = &f (key (), base_scope ()); // Already from the pool.
-      else
-        fail << "no default extension for target " << *this;
+      if (auto f = type ().extension)
+        ext = f (key (), base_scope ()); // Already from the pool.
+
+      if (ext == nullptr)
+      {
+        if (de != nullptr)
+          ext = &extension_pool.find (de);
+        else
+          fail << "no default extension for target " << *this;
+      }
     }
 
     // Add the extension.
@@ -418,7 +422,13 @@ namespace build2
     return t;
   }
 
-  const string&
+  const string*
+  target_extension_null (const target_key&, scope&)
+  {
+    return nullptr;
+  }
+
+  const string*
   target_extension_fail (const target_key& tk, scope& s)
   {
     {
@@ -439,7 +449,7 @@ namespace build2
     throw failed ();
   }
 
-  const string&
+  const string*
   target_extension_assert (const target_key&, scope&)
   {
     assert (false); // Attempt to obtain the default extension.
@@ -556,13 +566,13 @@ namespace build2
     false
   };
 
-  static const std::string&
+  static const std::string*
   buildfile_target_extension (const target_key& tk, scope&)
   {
     // If the name is special 'buildfile', then there is no extension,
     // otherwise it is .build.
     //
-    return extension_pool.find (*tk.name == "buildfile" ? "" : "build");
+    return &extension_pool.find (*tk.name == "buildfile" ? "" : "build");
   }
 
   const target_type buildfile::static_type
diff --git a/build2/target.txx b/build2/target.txx
index fbaa8ef..4999f61 100644
--- a/build2/target.txx
+++ b/build2/target.txx
@@ -10,14 +10,14 @@
 namespace build2
 {
   template <const char* ext>
-  const string&
+  const string*
   target_extension_fix (const target_key&, scope&)
   {
-    return extension_pool.find (ext);
+    return &extension_pool.find (ext);
   }
 
   template <const char* var, const char* def>
-  const string&
+  const string*
   target_extension_var (const target_key& tk, scope& s)
   {
     // Include target type/pattern-specific variables.
@@ -27,29 +27,10 @@ namespace build2
       // Help the user here and strip leading '.' from the extension.
       //
       const string& e (as<string> (*l));
-      return extension_pool.find (
+      return &extension_pool.find (
         !e.empty () && e.front () == '.' ? string (e, 1) : e);
     }
 
-    if (def != nullptr)
-      return extension_pool.find (def);
-
-    // Similar code to target_extension_fail().
-    //
-    {
-      diag_record dr;
-      dr << error << "no default extension in variable '" << var << "'"
-         << info << "required to derive file name for ";
-
-      if (tk.dir->absolute ())
-        dr << "target " << tk;
-      else
-        dr << "prerequisite " << prerequisite_key {nullptr, tk, &s};
-
-      dr << info << "perhaps you forgot to add "
-         << tk.type->name << "{*}: " << var << " = ...";
-    }
-
-    throw failed ();
+    return def != nullptr ? &extension_pool.find (def) : nullptr;
   }
 }