cmTarget: factor out fileset type handling

This allows for new fileset types to be added more easily by factoring
out the declarative information into a structure.

Source/cmTarget.cxx

@@ -165,6 +165,66 @@ cmValue cmTargetPropertyComputer::GetSources<cmTarget>(cmTarget const* tgt,
   return cmValue(srcs);
 }
 
+namespace {
+struct FileSetEntries
+{
+  FileSetEntries(cm::static_string_view propertyName)
+    : PropertyName(propertyName)
+  {
+  }
+
+  cm::static_string_view const PropertyName;
+  std::vector<BT<std::string>> Entries;
+};
+
+struct FileSetType
+{
+  FileSetType(cm::static_string_view typeName,
+              cm::static_string_view defaultDirectoryProperty,
+              cm::static_string_view defaultPathProperty,
+              cm::static_string_view directoryPrefix,
+              cm::static_string_view pathPrefix,
+              cm::static_string_view typeDescription,
+              cm::static_string_view defaultDescription,
+              cm::static_string_view arbitraryDescription,
+              FileSetEntries selfEntries, FileSetEntries interfaceEntries)
+    : TypeName(typeName)
+    , DefaultDirectoryProperty(defaultDirectoryProperty)
+    , DefaultPathProperty(defaultPathProperty)
+    , DirectoryPrefix(directoryPrefix)
+    , PathPrefix(pathPrefix)
+    , TypeDescription(typeDescription)
+    , DefaultDescription(defaultDescription)
+    , ArbitraryDescription(arbitraryDescription)
+    , SelfEntries(std::move(selfEntries))
+    , InterfaceEntries(std::move(interfaceEntries))
+  {
+  }
+
+  cm::static_string_view const TypeName;
+  cm::static_string_view const DefaultDirectoryProperty;
+  cm::static_string_view const DefaultPathProperty;
+  cm::static_string_view const DirectoryPrefix;
+  cm::static_string_view const PathPrefix;
+  cm::static_string_view const TypeDescription;
+  cm::static_string_view const DefaultDescription;
+  cm::static_string_view const ArbitraryDescription;
+
+  FileSetEntries SelfEntries;
+  FileSetEntries InterfaceEntries;
+
+  template <typename ValueType>
+  bool WriteProperties(cmTarget* tgt, cmTargetInternals* impl,
+                       const std::string& prop, ValueType value, bool clear);
+  std::pair<bool, cmValue> ReadProperties(cmTarget const* tgt,
+                                          cmTargetInternals const* impl,
+                                          const std::string& prop) const;
+
+  void AddFileSet(const std::string& name, cmFileSetVisibility vis,
+                  cmListFileBacktrace bt);
+};
+}
+
 class cmTargetInternals
 {
 public:
@@ -206,13 +266,15 @@ public:
   std::vector<BT<std::string>> LinkInterfacePropertyEntries;
   std::vector<BT<std::string>> LinkInterfaceDirectPropertyEntries;
   std::vector<BT<std::string>> LinkInterfaceDirectExcludePropertyEntries;
-  std::vector<BT<std::string>> HeaderSetsEntries;
-  std::vector<BT<std::string>> InterfaceHeaderSetsEntries;
   std::vector<std::pair<cmTarget::TLLSignature, cmListFileContext>>
     TLLCommands;
   std::map<std::string, cmFileSet> FileSets;
   cmListFileBacktrace Backtrace;
 
+  FileSetType HeadersFileSets;
+
+  cmTargetInternals();
+
   bool CheckImportedLibName(std::string const& prop,
                             std::string const& value) const;
 
@@ -233,6 +295,123 @@ public:
                           cm::string_view fileSetType) const;
 };
 
+cmTargetInternals::cmTargetInternals()
+  : HeadersFileSets("HEADERS"_s, "HEADER_DIRS"_s, "HEADER_SET"_s,
+                    "HEADER_DIRS_"_s, "HEADER_SET_"_s, "Header"_s,
+                    "The default header set"_s, "Header set"_s,
+                    FileSetEntries("HEADER_SETS"_s),
+                    FileSetEntries("INTERFACE_HEADER_SETS"_s))
+{
+}
+
+template <typename ValueType>
+bool FileSetType::WriteProperties(cmTarget* tgt, cmTargetInternals* impl,
+                                  const std::string& prop, ValueType value,
+                                  bool clear)
+{
+  if (prop == this->DefaultDirectoryProperty) {
+    impl->AddDirectoryToFileSet(tgt, std::string(this->TypeName), value,
+                                this->TypeName, this->DefaultDescription,
+                                clear);
+    return true;
+  }
+  if (prop == this->DefaultPathProperty) {
+    impl->AddPathToFileSet(tgt, std::string(this->TypeName), value,
+                           this->TypeName, this->DefaultDescription, clear);
+    return true;
+  }
+  if (cmHasPrefix(prop, this->DirectoryPrefix)) {
+    auto fileSetName = prop.substr(this->DirectoryPrefix.size());
+    if (fileSetName.empty()) {
+      impl->Makefile->IssueMessage(
+        MessageType::FATAL_ERROR,
+        cmStrCat(this->ArbitraryDescription, " name cannot be empty."));
+    } else {
+      impl->AddDirectoryToFileSet(
+        tgt, fileSetName, value, this->TypeName,
+        cmStrCat(this->ArbitraryDescription, " \"", fileSetName, "\""), clear);
+    }
+    return true;
+  }
+  if (cmHasPrefix(prop, this->PathPrefix)) {
+    auto fileSetName = prop.substr(this->PathPrefix.size());
+    if (fileSetName.empty()) {
+      impl->Makefile->IssueMessage(
+        MessageType::FATAL_ERROR,
+        cmStrCat(this->ArbitraryDescription, " name cannot be empty."));
+    } else {
+      impl->AddPathToFileSet(
+        tgt, fileSetName, value, this->TypeName,
+        cmStrCat(this->ArbitraryDescription, " \"", fileSetName, "\""), clear);
+    }
+    return true;
+  }
+  if (prop == this->SelfEntries.PropertyName) {
+    impl->Makefile->IssueMessage(
+      MessageType::FATAL_ERROR,
+      cmStrCat(this->SelfEntries.PropertyName, " property is read-only\n"));
+    return true;
+  }
+  if (prop == this->InterfaceEntries.PropertyName) {
+    impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
+                                 cmStrCat(this->InterfaceEntries.PropertyName,
+                                          " property is read-only\n"));
+    return true;
+  }
+  return false;
+}
+
+std::pair<bool, cmValue> FileSetType::ReadProperties(
+  cmTarget const* tgt, cmTargetInternals const* impl,
+  const std::string& prop) const
+{
+  bool did_read = false;
+  cmValue value = nullptr;
+  if (prop == this->DefaultDirectoryProperty) {
+    value = impl->GetFileSetDirectories(tgt, std::string(this->TypeName),
+                                        this->TypeName);
+    did_read = true;
+  } else if (prop == this->DefaultPathProperty) {
+    value =
+      impl->GetFileSetPaths(tgt, std::string(this->TypeName), this->TypeName);
+    did_read = true;
+  } else if (prop == this->SelfEntries.PropertyName) {
+    static std::string output;
+    output = cmJoin(this->SelfEntries.Entries, ";"_s);
+    value = cmValue(output);
+    did_read = true;
+  } else if (prop == this->InterfaceEntries.PropertyName) {
+    static std::string output;
+    output = cmJoin(this->InterfaceEntries.Entries, ";"_s);
+    value = cmValue(output);
+    did_read = true;
+  } else if (cmHasPrefix(prop, this->DirectoryPrefix)) {
+    std::string fileSetName = prop.substr(this->DirectoryPrefix.size());
+    if (!fileSetName.empty()) {
+      value = impl->GetFileSetDirectories(tgt, fileSetName, this->TypeName);
+    }
+    did_read = true;
+  } else if (cmHasPrefix(prop, this->PathPrefix)) {
+    std::string fileSetName = prop.substr(this->PathPrefix.size());
+    if (!fileSetName.empty()) {
+      value = impl->GetFileSetPaths(tgt, fileSetName, this->TypeName);
+    }
+    did_read = true;
+  }
+  return { did_read, value };
+}
+
+void FileSetType::AddFileSet(const std::string& name, cmFileSetVisibility vis,
+                             cmListFileBacktrace bt)
+{
+  if (cmFileSetVisibilityIsForSelf(vis)) {
+    this->SelfEntries.Entries.emplace_back(name, bt);
+  }
+  if (cmFileSetVisibilityIsForInterface(vis)) {
+    this->InterfaceEntries.Entries.emplace_back(name, std::move(bt));
+  }
+}
+
 namespace {
 #define SETUP_COMMON_LANGUAGE_PROPERTIES(lang)                                \
   initProp(#lang "_COMPILER_LAUNCHER");                                       \
@@ -1177,12 +1356,12 @@ cmBTStringRange cmTarget::GetLinkInterfaceDirectExcludeEntries() const
 
 cmBTStringRange cmTarget::GetHeaderSetsEntries() const
 {
-  return cmMakeRange(this->impl->HeaderSetsEntries);
+  return cmMakeRange(this->impl->HeadersFileSets.SelfEntries.Entries);
 }
 
 cmBTStringRange cmTarget::GetInterfaceHeaderSetsEntries() const
 {
-  return cmMakeRange(this->impl->InterfaceHeaderSetsEntries);
+  return cmMakeRange(this->impl->HeadersFileSets.InterfaceEntries.Entries);
 }
 
 namespace {
@@ -1214,10 +1393,6 @@ MAKE_PROP(BINARY_DIR);
 MAKE_PROP(SOURCE_DIR);
 MAKE_PROP(FALSE);
 MAKE_PROP(TRUE);
-MAKE_PROP(HEADER_DIRS);
-MAKE_PROP(HEADER_SET);
-MAKE_PROP(HEADER_SETS);
-MAKE_PROP(INTERFACE_HEADER_SETS);
 MAKE_PROP(INTERFACE_LINK_LIBRARIES);
 MAKE_PROP(INTERFACE_LINK_LIBRARIES_DIRECT);
 MAKE_PROP(INTERFACE_LINK_LIBRARIES_DIRECT_EXCLUDE);
@@ -1392,7 +1567,9 @@ void cmTarget::StoreProperty(const std::string& prop, ValueType value)
     }
   } else if (cmHasLiteralPrefix(prop, "IMPORTED_LIBNAME") &&
              !this->impl->CheckImportedLibName(
-               prop, value ? value : std::string{})) {
+               prop,
+               value ? value
+                     : std::string{})) { // NOLINT(bugprone-branch-clone)
     /* error was reported by check method */
   } else if (prop == propCUDA_PTX_COMPILATION &&
              this->GetType() != cmStateEnums::OBJECT_LIBRARY) {
@@ -1443,41 +1620,9 @@ void cmTarget::StoreProperty(const std::string& prop, ValueType value)
     } else {
       this->impl->LanguageStandardProperties.erase(prop);
     }
-  } else if (prop == propHEADER_DIRS) {
+  } else if (this->impl->HeadersFileSets.WriteProperties(
-    this->impl->AddDirectoryToFileSet(this, "HEADERS", value, "HEADERS"_s,
+               this, this->impl.get(), prop, value, true)) {
-                                      "The default header set"_s, true);
+    /* Handled in the `if` condition. */
-  } else if (prop == propHEADER_SET) {
-    this->impl->AddPathToFileSet(this, "HEADERS", value, "HEADERS"_s,
-                                 "The default header set"_s, true);
-  } else if (cmHasLiteralPrefix(prop, "HEADER_DIRS_")) {
-    auto fileSetName = prop.substr(cmStrLen("HEADER_DIRS_"));
-    if (fileSetName.empty()) {
-      this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                         "Header set name cannot be empty.");
-      return;
-    }
-    this->impl->AddDirectoryToFileSet(
-      this, fileSetName, value, "HEADERS"_s,
-      cmStrCat("Header set \"", fileSetName, "\""), true);
-  } else if (cmHasLiteralPrefix(prop, "HEADER_SET_")) {
-    auto fileSetName = prop.substr(cmStrLen("HEADER_SET_"));
-    if (fileSetName.empty()) {
-      this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                         "Header set name cannot be empty.");
-      return;
-    }
-    this->impl->AddPathToFileSet(this, fileSetName, value, "HEADERS"_s,
-                                 cmStrCat("Header set \"", fileSetName, "\""),
-                                 true);
-  } else if (prop == propHEADER_SETS) {
-    this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                       "HEADER_SETS property is read-only\n");
-    return;
-  } else if (prop == propINTERFACE_HEADER_SETS) {
-    this->impl->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      "INTERFACE_HEADER_SETS property is read-only\n");
-    return;
   } else {
     this->impl->Properties.SetProperty(prop, value);
   }
@@ -1588,41 +1733,9 @@ void cmTarget::AppendProperty(const std::string& prop,
              prop == "OBJC_STANDARD" || prop == "OBJCXX_STANDARD") {
     this->impl->Makefile->IssueMessage(
       MessageType::FATAL_ERROR, prop + " property may not be appended.");
-  } else if (prop == "HEADER_DIRS") {
+  } else if (this->impl->HeadersFileSets.WriteProperties(
-    this->impl->AddDirectoryToFileSet(this, "HEADERS", value, "HEADERS"_s,
+               this, this->impl.get(), prop, value, false)) {
-                                      "The default header set"_s, false);
+    /* Handled in the `if` condition. */
-  } else if (cmHasLiteralPrefix(prop, "HEADER_DIRS_")) {
-    auto fileSetName = prop.substr(cmStrLen("HEADER_DIRS_"));
-    if (fileSetName.empty()) {
-      this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                         "Header set name cannot be empty.");
-      return;
-    }
-    this->impl->AddDirectoryToFileSet(
-      this, fileSetName, value, "HEADERS"_s,
-      cmStrCat("Header set \"", fileSetName, "\""), false);
-  } else if (prop == "HEADER_SET") {
-    this->impl->AddPathToFileSet(this, "HEADERS", value, "HEADERS"_s,
-                                 "The default header set"_s, false);
-  } else if (cmHasLiteralPrefix(prop, "HEADER_SET_")) {
-    auto fileSetName = prop.substr(cmStrLen("HEADER_SET_"));
-    if (fileSetName.empty()) {
-      this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                         "Header set name cannot be empty.");
-      return;
-    }
-    this->impl->AddPathToFileSet(this, fileSetName, value, "HEADERS"_s,
-                                 cmStrCat("Header set \"", fileSetName, "\""),
-                                 false);
-  } else if (prop == "HEADER_SETS") {
-    this->impl->Makefile->IssueMessage(MessageType::FATAL_ERROR,
-                                       "HEADER_SETS property is read-only\n");
-    return;
-  } else if (prop == "INTERFACE_HEADER_SETS") {
-    this->impl->Makefile->IssueMessage(
-      MessageType::FATAL_ERROR,
-      "INTERFACE_HEADER_SETS property is read-only\n");
-    return;
   } else {
     this->impl->Properties.AppendProperty(prop, value, asString);
   }
@@ -1964,10 +2077,6 @@ cmValue cmTarget::GetProperty(const std::string& prop) const
     propBINARY_DIR,
     propSOURCE_DIR,
     propSOURCES,
-    propHEADER_DIRS,
-    propHEADER_SET,
-    propHEADER_SETS,
-    propINTERFACE_HEADER_SETS,
     propINTERFACE_LINK_LIBRARIES,
     propINTERFACE_LINK_LIBRARIES_DIRECT,
     propINTERFACE_LINK_LIBRARIES_DIRECT_EXCLUDE,
@@ -2124,49 +2233,15 @@ cmValue cmTarget::GetProperty(const std::string& prop) const
                        .GetDirectory()
                        .GetCurrentSource());
     }
-    if (prop == propHEADER_DIRS) {
-      return this->impl->GetFileSetDirectories(this, "HEADERS", "HEADERS"_s);
-    }
-    if (prop == propHEADER_SET) {
-      return this->impl->GetFileSetPaths(this, "HEADERS", "HEADERS"_s);
-    }
-    if (prop == propHEADER_SETS) {
-      std::vector<std::string> set_names;
-      for (auto const& file_set : this->impl->FileSets) {
-        if (cmFileSetVisibilityIsForSelf(file_set.second.GetVisibility())) {
-          set_names.push_back(file_set.second.GetName());
-        }
-      }
-      static std::string output;
-      output = cmJoin(set_names, ";"_s);
-      return cmValue(output);
-    }
-    if (prop == propINTERFACE_HEADER_SETS) {
-      std::vector<std::string> set_names;
-      for (auto const& file_set : this->impl->FileSets) {
-        if (cmFileSetVisibilityIsForInterface(
-              file_set.second.GetVisibility())) {
-          set_names.push_back(file_set.second.GetName());
-        }
-      }
-      static std::string output;
-      output = cmJoin(set_names, ";"_s);
-      return cmValue(output);
-    }
   }
-  if (cmHasLiteralPrefix(prop, "HEADER_DIRS_")) {
+
-    std::string fileSetName = prop.substr(cmStrLen("HEADER_DIRS_"));
+  // Check fileset properties.
-    if (fileSetName.empty()) {
+  {
-      return nullptr;
+    auto headers =
+      this->impl->HeadersFileSets.ReadProperties(this, this->impl.get(), prop);
+    if (headers.first) {
+      return headers.second;
     }
-    return this->impl->GetFileSetDirectories(this, fileSetName, "HEADERS"_s);
-  }
-  if (cmHasLiteralPrefix(prop, "HEADER_SET_")) {
-    std::string fileSetName = prop.substr(cmStrLen("HEADER_SET_"));
-    if (fileSetName.empty()) {
-      return nullptr;
-    }
-    return this->impl->GetFileSetPaths(this, fileSetName, "HEADERS"_s);
   }
 
   cmValue retVal = this->impl->Properties.GetPropertyValue(prop);
@@ -2441,13 +2516,9 @@ std::pair<cmFileSet*, bool> cmTarget::GetOrCreateFileSet(
   auto result = this->impl->FileSets.emplace(
     std::make_pair(name, cmFileSet(name, type, vis)));
   if (result.second) {
-    if (cmFileSetVisibilityIsForSelf(vis)) {
+    auto bt = this->impl->Makefile->GetBacktrace();
-      this->impl->HeaderSetsEntries.emplace_back(
+    if (type == this->impl->HeadersFileSets.TypeName) {
-        name, this->impl->Makefile->GetBacktrace());
+      this->impl->HeadersFileSets.AddFileSet(name, vis, std::move(bt));
-    }
-    if (cmFileSetVisibilityIsForInterface(vis)) {
-      this->impl->InterfaceHeaderSetsEntries.emplace_back(
-        name, this->impl->Makefile->GetBacktrace());
     }
   }
   return std::make_pair(&result.first->second, result.second);
@@ -2481,7 +2552,7 @@ std::vector<std::string> cmTarget::GetAllInterfaceFileSets() const
     }
   };
 
-  appendEntries(this->impl->InterfaceHeaderSetsEntries);
+  appendEntries(this->impl->HeadersFileSets.InterfaceEntries.Entries);
 
   return result;
 }

Tests/RunCMake/target_sources/FileSetImport.cmake

@@ -17,7 +17,7 @@ include("${export_build_dir}/export.cmake")
 include("${export_build_dir}/install/lib/cmake/export.cmake")
 
 assert_prop_eq(export::lib1 HEADER_SETS "")
-assert_prop_eq(export::lib1 INTERFACE_HEADER_SETS "HEADERS;b;c;d;dir3;e;f;g")
+assert_prop_eq(export::lib1 INTERFACE_HEADER_SETS "HEADERS;b;c;d;e;f;g;dir3")
 assert_prop_eq(export::lib1 HEADER_SET "${CMAKE_CURRENT_SOURCE_DIR}/error.c")
 assert_prop_eq(export::lib1 HEADER_SET_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}/error.c")
 if (_multi_config)
@@ -72,7 +72,7 @@ else ()
 endif ()
 
 assert_prop_eq(install::lib1 HEADER_SETS "")
-assert_prop_eq(install::lib1 INTERFACE_HEADER_SETS "HEADERS;b;c;d;dir3;e;f;g")
+assert_prop_eq(install::lib1 INTERFACE_HEADER_SETS "HEADERS;b;c;d;e;f;g;dir3")
 assert_prop_eq(install::lib1 HEADER_SET "${export_build_dir}/install/include/error.c")
 assert_prop_eq(install::lib1 HEADER_DIRS "${export_build_dir}/install/include")
 assert_prop_eq(install::lib1 HEADER_SET_HEADERS "${export_build_dir}/install/include/error.c")

Tests/RunCMake/target_sources/FileSetProperties.cmake

@@ -57,7 +57,7 @@ assert_prop_eq(lib1 INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURC
 assert_prop_eq(lib1 INTERFACE_INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/.>;$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>;$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>")
 
 target_sources(lib1 PUBLIC FILE_SET HEADERS BASE_DIRS "${CMAKE_CURRENT_SOURCE_DIR}" FILES h1.h)
-assert_prop_eq(lib1 INTERFACE_HEADER_SETS "HEADERS;a;c;d")
+assert_prop_eq(lib1 INTERFACE_HEADER_SETS "a;c;d;HEADERS")
 assert_prop_eq(lib1 HEADER_DIRS "${CMAKE_CURRENT_SOURCE_DIR}")
 assert_prop_eq(lib1 HEADER_SET "${CMAKE_CURRENT_SOURCE_DIR}/h1.h")
 assert_prop_eq(lib1 HEADER_DIRS_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}")
@@ -66,7 +66,7 @@ assert_prop_eq(lib1 INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURC
 assert_prop_eq(lib1 INTERFACE_INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/.>;$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>;$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>;$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>")
 
 target_sources(lib1 PUBLIC FILE_SET HEADERS FILES h2.h)
-assert_prop_eq(lib1 INTERFACE_HEADER_SETS "HEADERS;a;c;d")
+assert_prop_eq(lib1 INTERFACE_HEADER_SETS "a;c;d;HEADERS")
 assert_prop_eq(lib1 HEADER_DIRS "${CMAKE_CURRENT_SOURCE_DIR}")
 assert_prop_eq(lib1 HEADER_SET "${CMAKE_CURRENT_SOURCE_DIR}/h1.h;${CMAKE_CURRENT_SOURCE_DIR}/h2.h")
 assert_prop_eq(lib1 HEADER_DIRS_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}")