Don't duplicate -D defines sent to the compiler.

There is no need to do so. Be consistent with include directories and
ensure uniqueness.

This requires changing the API of the cmLocalGenerator::AppendDefines
method, and changing the generators to match.

The test unfortunately can't test for uniqueness, but it at least verifies
that nothing gets lost.

Source/cmLocalGenerator.cxx

@@ -2101,9 +2101,8 @@ void cmLocalGenerator::AppendFlags(std::string& flags,
 }
 
 //----------------------------------------------------------------------------
-void cmLocalGenerator::AppendDefines(std::string& defines,
+void cmLocalGenerator::AppendDefines(std::set<std::string>& defines,
-                                     const char* defines_list,
+                                     const char* defines_list)
-                                     const char* lang)
 {
   // Short-circuit if there are no definitions.
   if(!defines_list)
@@ -2115,12 +2114,23 @@ void cmLocalGenerator::AppendDefines(std::string& defines,
   std::vector<std::string> defines_vec;
   cmSystemTools::ExpandListArgument(defines_list, defines_vec);
 
-  // Short-circuit if there are no definitions.
+  for(std::vector<std::string>::const_iterator di = defines_vec.begin();
-  if(defines_vec.empty())
+      di != defines_vec.end(); ++di)
     {
-    return;
+    // Skip unsupported definitions.
+    if(!this->CheckDefinition(*di))
+      {
+      continue;
+      }
+    defines.insert(*di);
     }
+}
 
+//----------------------------------------------------------------------------
+void cmLocalGenerator::JoinDefines(const std::set<std::string>& defines,
+                                   std::string &definesString,
+                                   const char* lang)
+{
   // Lookup the define flag for the current language.
   std::string dflag = "-D";
   if(lang)
@@ -2135,23 +2145,13 @@ void cmLocalGenerator::AppendDefines(std::string& defines,
       }
     }
 
-  // Add each definition to the command line with appropriate escapes.
+  std::set<std::string>::const_iterator defineIt = defines.begin();
-  const char* dsep = defines.empty()? "" : " ";
+  const std::set<std::string>::const_iterator defineEnd = defines.end();
-  for(std::vector<std::string>::const_iterator di = defines_vec.begin();
+  const char* itemSeparator = definesString.empty() ? "" : " ";
-      di != defines_vec.end(); ++di)
+  for( ; defineIt != defineEnd; ++defineIt)
     {
-    // Skip unsupported definitions.
-    if(!this->CheckDefinition(*di))
-      {
-      continue;
-      }
-
-    // Separate from previous definitions.
-    defines += dsep;
-    dsep = " ";
-
     // Append the definition with proper escaping.
-    defines += dflag;
+    std::string def = dflag;
     if(this->WatcomWMake)
       {
       // The Watcom compiler does its own command line parsing instead
@@ -2164,27 +2164,30 @@ void cmLocalGenerator::AppendDefines(std::string& defines,
       // command line without any escapes.  However we still have to
       // get the '$' and '#' characters through WMake as '$$' and
       // '$#'.
-      for(const char* c = di->c_str(); *c; ++c)
+      for(const char* c = defineIt->c_str(); *c; ++c)
         {
         if(*c == '$' || *c == '#')
           {
-          defines += '$';
+          def += '$';
           }
-        defines += *c;
+        def += *c;
         }
       }
     else
       {
       // Make the definition appear properly on the command line.  Use
       // -DNAME="value" instead of -D"NAME=value" to help VS6 parser.
-      std::string::size_type eq = di->find("=");
+      std::string::size_type eq = defineIt->find("=");
-      defines += di->substr(0, eq);
+      def += defineIt->substr(0, eq);
-      if(eq != di->npos)
+      if(eq != defineIt->npos)
         {
-        defines += "=";
+        def += "=";
-        defines += this->EscapeForShell(di->c_str() + eq + 1, true);
+        def += this->EscapeForShell(defineIt->c_str() + eq + 1, true);
         }
       }
+    definesString += itemSeparator;
+    itemSeparator = " ";
+    definesString += def;
     }
 }
 

Source/cmLocalGenerator.h

@@ -154,8 +154,14 @@ public:
    * Encode a list of preprocessor definitions for the compiler
    * command line.
    */
-  void AppendDefines(std::string& defines, const char* defines_list,
+  void AppendDefines(std::set<std::string>& defines,
-                     const char* lang);
+                     const char* defines_list);
+  /**
+   * Join a set of defines into a definesString with a space separator.
+   */
+  void JoinDefines(const std::set<std::string>& defines,
+                   std::string &definesString,
+                   const char* lang);
 
   /** Lookup and append options associated with a particular feature.  */
   void AppendFeatureOptions(std::string& flags, const char* lang,

Source/cmLocalVisualStudio6Generator.cxx

@@ -418,25 +418,42 @@ void cmLocalVisualStudio6Generator
 
     // Add per-source and per-configuration preprocessor definitions.
     std::map<cmStdString, cmStdString> cdmap;
-    this->AppendDefines(compileFlags,
+
-                        (*sf)->GetProperty("COMPILE_DEFINITIONS"), lang);
+      {
+      std::set<std::string> targetCompileDefinitions;
+
+      this->AppendDefines(targetCompileDefinitions,
+                        (*sf)->GetProperty("COMPILE_DEFINITIONS"));
+      this->JoinDefines(targetCompileDefinitions, compileFlags, lang);
+      }
+
     if(const char* cdefs = (*sf)->GetProperty("COMPILE_DEFINITIONS_DEBUG"))
       {
-      this->AppendDefines(cdmap["DEBUG"], cdefs, lang);
+      std::set<std::string> debugCompileDefinitions;
+      this->AppendDefines(debugCompileDefinitions, cdefs);
+      this->JoinDefines(debugCompileDefinitions, cdmap["DEBUG"], lang);
       }
     if(const char* cdefs = (*sf)->GetProperty("COMPILE_DEFINITIONS_RELEASE"))
       {
-      this->AppendDefines(cdmap["RELEASE"], cdefs, lang);
+      std::set<std::string> releaseCompileDefinitions;
+      this->AppendDefines(releaseCompileDefinitions, cdefs);
+      this->JoinDefines(releaseCompileDefinitions, cdmap["RELEASE"], lang);
       }
     if(const char* cdefs =
        (*sf)->GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"))
       {
-      this->AppendDefines(cdmap["MINSIZEREL"], cdefs, lang);
+      std::set<std::string> minsizerelCompileDefinitions;
+      this->AppendDefines(minsizerelCompileDefinitions, cdefs);
+      this->JoinDefines(minsizerelCompileDefinitions, cdmap["MINSIZEREL"],
+                        lang);
       }
     if(const char* cdefs =
        (*sf)->GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"))
       {
-      this->AppendDefines(cdmap["RELWITHDEBINFO"], cdefs, lang);
+      std::set<std::string> relwithdebinfoCompileDefinitions;
+      this->AppendDefines(relwithdebinfoCompileDefinitions, cdefs);
+      this->JoinDefines(relwithdebinfoCompileDefinitions,
+                        cdmap["RELWITHDEBINFO"], lang);
       }
 
     bool excludedFromBuild =
@@ -1653,43 +1670,56 @@ void cmLocalVisualStudio6Generator
       }
 
     // Add per-target and per-configuration preprocessor definitions.
+    std::set<std::string> definesSet;
+    std::set<std::string> debugDefinesSet;
+    std::set<std::string> releaseDefinesSet;
+    std::set<std::string> minsizeDefinesSet;
+    std::set<std::string> debugrelDefinesSet;
+
+    this->AppendDefines(
+      definesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS"));
+    this->AppendDefines(
+      debugDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_DEBUG"));
+    this->AppendDefines(
+      releaseDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELEASE"));
+    this->AppendDefines(
+      minsizeDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"));
+    this->AppendDefines(
+      debugrelDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"));
+
+    this->AppendDefines(
+      definesSet,
+      target.GetProperty("COMPILE_DEFINITIONS"));
+    this->AppendDefines(
+      debugDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_DEBUG"));
+    this->AppendDefines(
+      releaseDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_RELEASE"));
+    this->AppendDefines(
+      minsizeDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"));
+    this->AppendDefines(
+      debugrelDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"));
+
     std::string defines = " ";
     std::string debugDefines = " ";
     std::string releaseDefines = " ";
     std::string minsizeDefines = " ";
     std::string debugrelDefines = " ";
 
-    this->AppendDefines(
+    this->JoinDefines(definesSet, defines, 0);
-      defines,
+    this->JoinDefines(debugDefinesSet, debugDefines, 0);
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS"), 0);
+    this->JoinDefines(releaseDefinesSet, releaseDefines, 0);
-    this->AppendDefines(
+    this->JoinDefines(minsizeDefinesSet, minsizeDefines, 0);
-      debugDefines,
+    this->JoinDefines(debugrelDefinesSet, debugrelDefines, 0);
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_DEBUG"),0);
-    this->AppendDefines(
-      releaseDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELEASE"), 0);
-    this->AppendDefines(
-      minsizeDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"), 0);
-    this->AppendDefines(
-      debugrelDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"), 0);
 
-    this->AppendDefines(
-      defines,
-      target.GetProperty("COMPILE_DEFINITIONS"), 0);
-    this->AppendDefines(
-      debugDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_DEBUG"), 0);
-    this->AppendDefines(
-      releaseDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_RELEASE"), 0);
-    this->AppendDefines(
-      minsizeDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"), 0);
-    this->AppendDefines(
-      debugrelDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"), 0);
     flags += defines;
     flagsDebug += debugDefines;
     flagsRelease += releaseDefines;

Source/cmMakefileTargetGenerator.cxx

@@ -292,28 +292,31 @@ std::string cmMakefileTargetGenerator::GetDefines(const std::string &l)
   ByLanguageMap::iterator i = this->DefinesByLanguage.find(l);
   if (i == this->DefinesByLanguage.end())
     {
-    std::string defines;
+    std::set<std::string> defines;
     const char *lang = l.c_str();
     // Add the export symbol definition for shared library objects.
     if(const char* exportMacro = this->Target->GetExportMacro())
       {
-      this->LocalGenerator->AppendDefines(defines, exportMacro, lang);
+      this->LocalGenerator->AppendDefines(defines, exportMacro);
       }
 
     // Add preprocessor definitions for this target and configuration.
     this->LocalGenerator->AppendDefines
-      (defines, this->Makefile->GetProperty("COMPILE_DEFINITIONS"), lang);
+      (defines, this->Makefile->GetProperty("COMPILE_DEFINITIONS"));
     this->LocalGenerator->AppendDefines
-      (defines, this->Target->GetProperty("COMPILE_DEFINITIONS"), lang);
+      (defines, this->Target->GetProperty("COMPILE_DEFINITIONS"));
     std::string defPropName = "COMPILE_DEFINITIONS_";
     defPropName +=
       cmSystemTools::UpperCase(this->LocalGenerator->ConfigurationName);
     this->LocalGenerator->AppendDefines
-      (defines, this->Makefile->GetProperty(defPropName.c_str()), lang);
+      (defines, this->Makefile->GetProperty(defPropName.c_str()));
     this->LocalGenerator->AppendDefines
-      (defines, this->Target->GetProperty(defPropName.c_str()), lang);
+      (defines, this->Target->GetProperty(defPropName.c_str()));
 
-    ByLanguageMap::value_type entry(l, defines);
+    std::string definesString;
+    this->LocalGenerator->JoinDefines(defines, definesString, lang);
+
+    ByLanguageMap::value_type entry(l, definesString);
     i = this->DefinesByLanguage.insert(entry).first;
     }
   return i->second;
@@ -587,14 +590,12 @@ cmMakefileTargetGenerator
     }
 
   // Add language-specific defines.
-  std::string defines = "$(";
+  std::set<std::string> defines;
-  defines += lang;
-  defines += "_DEFINES)";
 
   // Add source-sepcific preprocessor definitions.
   if(const char* compile_defs = source.GetProperty("COMPILE_DEFINITIONS"))
     {
-    this->LocalGenerator->AppendDefines(defines, compile_defs, lang);
+    this->LocalGenerator->AppendDefines(defines, compile_defs);
     *this->FlagFileStream << "# Custom defines: "
                           << relativeObj << "_DEFINES = "
                           << compile_defs << "\n"
@@ -607,7 +608,7 @@ cmMakefileTargetGenerator
   if(const char* config_compile_defs =
      source.GetProperty(defPropName.c_str()))
     {
-    this->LocalGenerator->AppendDefines(defines, config_compile_defs, lang);
+    this->LocalGenerator->AppendDefines(defines, config_compile_defs);
     *this->FlagFileStream
       << "# Custom defines: "
       << relativeObj << "_DEFINES_" << configUpper
@@ -676,7 +677,14 @@ cmMakefileTargetGenerator
                             cmLocalGenerator::SHELL);
   vars.ObjectDir = objectDir.c_str();
   vars.Flags = flags.c_str();
-  vars.Defines = defines.c_str();
+
+  std::string definesString = "$(";
+  definesString += lang;
+  definesString += "_DEFINES)";
+
+  this->LocalGenerator->JoinDefines(defines, definesString, lang);
+
+  vars.Defines = definesString.c_str();
 
   bool lang_is_c_or_cxx = ((strcmp(lang, "C") == 0) ||
                            (strcmp(lang, "CXX") == 0));

Source/cmNinjaTargetGenerator.cxx

@@ -184,46 +184,43 @@ std::string
 cmNinjaTargetGenerator::
 ComputeDefines(cmSourceFile *source, const std::string& language)
 {
-  std::string defines;
+  std::set<std::string> defines;
 
   // Add the export symbol definition for shared library objects.
   if(const char* exportMacro = this->Target->GetExportMacro())
     {
-    this->LocalGenerator->AppendDefines(defines, exportMacro,
+    this->LocalGenerator->AppendDefines(defines, exportMacro);
-                                        language.c_str());
     }
 
   // Add preprocessor definitions for this target and configuration.
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Makefile->GetProperty("COMPILE_DEFINITIONS"),
+     this->Makefile->GetProperty("COMPILE_DEFINITIONS"));
-     language.c_str());
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Target->GetProperty("COMPILE_DEFINITIONS"),
+     this->Target->GetProperty("COMPILE_DEFINITIONS"));
-     language.c_str());
   this->LocalGenerator->AppendDefines
     (defines,
-     source->GetProperty("COMPILE_DEFINITIONS"),
+     source->GetProperty("COMPILE_DEFINITIONS"));
-     language.c_str());
   {
   std::string defPropName = "COMPILE_DEFINITIONS_";
   defPropName += cmSystemTools::UpperCase(this->GetConfigName());
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Makefile->GetProperty(defPropName.c_str()),
+     this->Makefile->GetProperty(defPropName.c_str()));
-     language.c_str());
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Target->GetProperty(defPropName.c_str()),
+     this->Target->GetProperty(defPropName.c_str()));
-     language.c_str());
   this->LocalGenerator->AppendDefines
     (defines,
-     source->GetProperty(defPropName.c_str()),
+     source->GetProperty(defPropName.c_str()));
-     language.c_str());
   }
 
-  return defines;
+  std::string definesString;
+  this->LocalGenerator->JoinDefines(defines, definesString,
+     language.c_str());
+
+  return definesString;
 }
 
 cmNinjaDeps cmNinjaTargetGenerator::ComputeLinkDeps() const

Tests/CMakeLists.txt

@@ -218,6 +218,7 @@ if(BUILD_TESTING)
   ADD_TEST_MACRO(Unset Unset)
   ADD_TEST_MACRO(PolicyScope PolicyScope)
   ADD_TEST_MACRO(EmptyLibrary EmptyLibrary)
+  ADD_TEST_MACRO(CompileDefinitions CompileDefinitions)
   set_tests_properties(EmptyLibrary PROPERTIES
     PASS_REGULAR_EXPRESSION "CMake Error: CMake can not determine linker language for target:test")
   ADD_TEST_MACRO(CrossCompile CrossCompile)

Tests/CompileDefinitions/CMakeLists.txt

@@ -0,0 +1,12 @@
+
+cmake_minimum_required(VERSION 2.8)
+
+project(CompileDefinitions)
+
+add_subdirectory(add_definitions_command)
+add_subdirectory(target_prop)
+add_subdirectory(add_definitions_command_with_target_prop)
+
+file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/main.cpp" "int main(int, char **) { return 0; }\n")
+
+add_executable(CompileDefinitions "${CMAKE_CURRENT_BINARY_DIR}/main.cpp")

Tests/CompileDefinitions/add_definitions_command/CMakeLists.txt

@@ -0,0 +1,7 @@
+
+project(add_definitions_command)
+
+add_definitions(-DCMAKE_IS_FUN -DCMAKE_IS=Fun -DCMAKE_IS_="Fun" -DCMAKE_IS_REALLY="Very Fun")
+add_definitions(-DCMAKE_IS_="Fun" -DCMAKE_IS_REALLY="Very Fun" -DCMAKE_IS_FUN -DCMAKE_IS=Fun)
+
+add_executable(add_definitions_command_executable ../main.cpp)

Tests/CompileDefinitions/add_definitions_command_with_target_prop/CMakeLists.txt

@@ -0,0 +1,14 @@
+
+project(add_definitions_command_with_target_prop)
+
+add_definitions(-DCMAKE_IS_FUN -DCMAKE_IS=Fun)
+
+add_executable(add_definitions_command_with_target_prop_executable ../main.cpp)
+
+set_target_properties(add_definitions_command_with_target_prop_executable PROPERTIES COMPILE_DEFINITIONS CMAKE_IS_="Fun")
+
+set_property(TARGET add_definitions_command_with_target_prop_executable APPEND PROPERTY COMPILE_DEFINITIONS CMAKE_IS_REALLY="Very Fun")
+
+add_definitions(-DCMAKE_IS_FUN)
+
+set_property(TARGET add_definitions_command_with_target_prop_executable APPEND PROPERTY COMPILE_DEFINITIONS CMAKE_IS=Fun CMAKE_IS_="Fun")

Tests/CompileDefinitions/main.cpp

@@ -0,0 +1,28 @@
+
+#ifndef CMAKE_IS_FUN
+#error Expect CMAKE_IS_FUN definition
+#endif
+
+#if CMAKE_IS != Fun
+#error Expect CMAKE_IS=Fun definition
+#endif
+
+
+template<bool test>
+struct CMakeStaticAssert;
+
+template<>
+struct CMakeStaticAssert<true> {};
+
+static const char fun_string[] = CMAKE_IS_;
+static const char very_fun_string[] = CMAKE_IS_REALLY;
+
+enum {
+  StringLiteralTest1 = sizeof(CMakeStaticAssert<sizeof(CMAKE_IS_) == sizeof("Fun")>),
+  StringLiteralTest2 = sizeof(CMakeStaticAssert<sizeof(CMAKE_IS_REALLY) == sizeof("Very Fun")>)
+};
+
+int main(int argc, char **argv)
+{
+  return 0;
+}

Tests/CompileDefinitions/target_prop/CMakeLists.txt

@@ -0,0 +1,9 @@
+
+project(target_prop)
+
+add_executable(target_prop_executable ../main.cpp)
+
+set_target_properties(target_prop_executable PROPERTIES CMAKE_IS_FUN -DCMAKE_IS_REALLY="Very Fun")
+
+set_property(TARGET target_prop_executable APPEND PROPERTY COMPILE_DEFINITIONS CMAKE_IS_REALLY="Very Fun" CMAKE_IS=Fun)
+set_property(TARGET target_prop_executable APPEND PROPERTY COMPILE_DEFINITIONS CMAKE_IS_FUN CMAKE_IS_="Fun")