Clean default target type extension logic

5 files changed, 83 insertions, 22 deletions

diff --git a/libbuild2/in/target.cxx b/libbuild2/in/target.cxx
index 0affc04..d9bc8a7 100644
--- a/libbuild2/in/target.cxx
+++ b/libbuild2/in/target.cxx
@@ -43,14 +43,12 @@ namespace build2
       fail (l) << "pattern in in{} prerequisite" << endf;
     }
 
-    extern const char in_ext_def[] = ""; // No extension by default.
-
     const target_type in::static_type
     {
       "in",
       &file::static_type,
       &target_factory<in>,
-      &target_extension_fix<in_ext_def>,
+      &target_extension_none,
       nullptr, /* default_extension */   // Taken care of by search.
       &in_pattern,
       &target_print_1_ext_verb,          // Same as file.
diff --git a/libbuild2/scope.cxx b/libbuild2/scope.cxx
index e16e773..3b612f8 100644
--- a/libbuild2/scope.cxx
+++ b/libbuild2/scope.cxx
@@ -887,6 +887,7 @@ namespace build2
     dt->base = &base;
     dt->factory = &derived_tt_factory;
 
+#if 0
     // @@ 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
@@ -918,6 +919,43 @@ namespace build2
       dt->fixed_extension != nullptr
       ? &target_print_0_ext_verb  // Fixed extension, no use printing.
       : nullptr;                  // Normal.
+#endif
+
+    // An attempt to clarify the above mess:
+    //
+    // 1. If we have a "really fixed" extension (like man1{}) then we keep
+    //    it (including pattern and print functions).
+    //
+    // 2. Otherwise, we make it target_extension_var.
+    //
+    // Note that this still mis-fires for the following scenarios:
+    //
+    // file{} -- What if the user does not set the default extension expecting
+    //           similar semantics as file{} or man{} itself. Maybe explicit
+    //           via attribute (i.e., inherit from base)?
+    //
+    // @@ Get the fallback extension from base target_extension_var
+    //    somehow (we know the base target type so could just call it)?
+    //
+    if (ext)
+    {
+      if (dt->fixed_extension == nullptr                ||
+          dt->fixed_extension == &target_extension_none ||
+          dt->fixed_extension == &target_extension_must)
+      {
+        dt->fixed_extension = nullptr;
+        dt->default_extension = &target_extension_var<nullptr>;
+        dt->pattern = &target_pattern_var<nullptr>;
+        dt->print = nullptr;
+      }
+    }
+    else
+    {
+      dt->fixed_extension = nullptr;
+      dt->default_extension = nullptr;
+      dt->pattern = nullptr;
+      dt->print = nullptr;
+    }
 
     return root_extra->target_types.insert (name, move (dt));
   }
diff --git a/libbuild2/target-type.hxx b/libbuild2/target-type.hxx
index 913432e..5798766 100644
--- a/libbuild2/target-type.hxx
+++ b/libbuild2/target-type.hxx
@@ -22,10 +22,23 @@ namespace build2
   // type does not use extensions. Note that this is relied upon when deciding
   // whether to print the extension.
   //
-  // The fixed extension function should return the fixed extension (which can
-  // point to the key's ext member; note that for performance reasons we
-  // currently only verify the explicitly specified extension on target
-  // insersion -- see target_key comparison for details).
+  // If the fixed extension function is specified, then it means that this
+  // target type has a fixed extension (including the no-extension case) and
+  // this function should return such a fixed extension (which, if overriding
+  // by the user is allowed, can point to the key's ext member; note that for
+  // performance reasons we currently only verify the explicitly specified
+  // extension on target insersion -- see target_key comparison for details).
+  // It is called eraly, during the target insertion, in contrast to the
+  // default extension function described below (you would specify one or the
+  // other).
+  //
+  // Note that the fixed no-extension case that allows overriding by the user
+  // is used to implement the "if extension is not specified by the user then
+  // there is no extension" semantics of the file{} and similar target types.
+  // For such cases the target_extension_none() function should be used (we
+  // compare to it's address to detect target types with such semantics).
+  // Similarly, for cases where the user must specify the extension explicitly
+  // (e.g., man{}), use target_extension_must().
   //
   // The root scope argument to the fixed extension function may be NULL which
   // means the root scope is not known. A target type that relies on this must
diff --git a/libbuild2/target.cxx b/libbuild2/target.cxx
index a5061e3..6ba12b9 100644
--- a/libbuild2/target.cxx
+++ b/libbuild2/target.cxx
@@ -979,6 +979,23 @@ namespace build2
     return search_existing_file (t.ctx, pk);
   }
 
+  extern const char target_extension_none_[] = "";
+
+  const char*
+  target_extension_none (const target_key& k, const scope* s)
+  {
+    return target_extension_fix<target_extension_none_> (k, s);
+  }
+
+  const char*
+  target_extension_must (const target_key& tk, const scope*)
+  {
+    if (!tk.ext)
+      fail << tk.type->name << " target " << tk << " must include extension";
+
+    return tk.ext->c_str ();
+  }
+
   void
   target_print_0_ext_verb (ostream& os, const target_key& k)
   {
@@ -1037,14 +1054,12 @@ namespace build2
     false
   };
 
-  extern const char file_ext_def[] = "";
-
   const target_type file::static_type
   {
     "file",
     &path_target::static_type,
     &target_factory<file>,
-    &target_extension_fix<file_ext_def>,
+    &target_extension_none,
     nullptr, /* default_extension */
     nullptr, /* pattern */
     &target_print_1_ext_verb, // Print extension even at verbosity level 0.
@@ -1467,7 +1482,7 @@ namespace build2
     "doc",
     &file::static_type,
     &target_factory<doc>,
-    &target_extension_fix<file_ext_def>, // Same as file (no extension).
+    &target_extension_none,              // Same as file (no extension).
     nullptr, /* default_extension */
     nullptr, /* pattern */               // Same as file.
     &target_print_1_ext_verb,            // Same as file.
@@ -1480,7 +1495,7 @@ namespace build2
     "legal",
     &doc::static_type,
     &target_factory<legal>,
-    &target_extension_fix<file_ext_def>, // Same as file (no extension).
+    &target_extension_none,              // Same as file (no extension).
     nullptr, /* default_extension */
     nullptr, /* pattern */               // Same as file.
     &target_print_1_ext_verb,            // Same as file.
@@ -1488,21 +1503,12 @@ namespace build2
     false
   };
 
-  static const char*
-  man_extension (const target_key& tk, const scope*)
-  {
-    if (!tk.ext)
-      fail << "man target " << tk << " must include extension (man section)";
-
-    return tk.ext->c_str ();
-  }
-
   const target_type man::static_type
   {
     "man",
     &doc::static_type,
     &target_factory<man>,
-    &man_extension,           // Should be specified explicitly.
+    &target_extension_must,   // Should be specified explicitly.
     nullptr, /* default_extension */
     nullptr,
     &target_print_1_ext_verb, // Print extension even at verbosity level 0.
diff --git a/libbuild2/target.hxx b/libbuild2/target.hxx
index 49d4563..10abf16 100644
--- a/libbuild2/target.hxx
+++ b/libbuild2/target.hxx
@@ -1940,6 +1940,12 @@ namespace build2
                       string&, optional<string>&, const location&,
                       bool);
 
+  const char*
+  target_extension_none (const target_key&, const scope*);
+
+  const char*
+  target_extension_must (const target_key&, const scope*);
+
   // Get the extension from the `extension` variable or use the default if
   // none set. If the default is NULL, then return NULL.
   //