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.

11 files changed, 206 insertions, 91 deletions

diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx
index 0cfb36b..cf2af0a 100644
--- a/Source/cmLocalGenerator.cxx
+++ b/Source/cmLocalGenerator.cxx
@@ -2101,9 +2101,8 @@ void cmLocalGenerator::AppendFlags(std::string& flags,
 }
 
 //----------------------------------------------------------------------------
-void cmLocalGenerator::AppendDefines(std::string& defines,
-                                     const char* defines_list,
-                                     const char* lang)
+void cmLocalGenerator::AppendDefines(std::set<std::string>& defines,
+                                     const char* defines_list)
 {
   // 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.
-  if(defines_vec.empty())
+  for(std::vector<std::string>::const_iterator di = defines_vec.begin();
+      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.
-  const char* dsep = defines.empty()? "" : " ";
-  for(std::vector<std::string>::const_iterator di = defines_vec.begin();
-      di != defines_vec.end(); ++di)
+  std::set<std::string>::const_iterator defineIt = defines.begin();
+  const std::set<std::string>::const_iterator defineEnd = defines.end();
+  const char* itemSeparator = definesString.empty() ? "" : " ";
+  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("=");
-      defines += di->substr(0, eq);
-      if(eq != di->npos)
+      std::string::size_type eq = defineIt->find("=");
+      def += defineIt->substr(0, eq);
+      if(eq != defineIt->npos)
         {
-        defines += "=";
-        defines += this->EscapeForShell(di->c_str() + eq + 1, true);
+        def += "=";
+        def += this->EscapeForShell(defineIt->c_str() + eq + 1, true);
         }
       }
+    definesString += itemSeparator;
+    itemSeparator = " ";
+    definesString += def;
     }
 }
 
diff --git a/Source/cmLocalGenerator.h b/Source/cmLocalGenerator.h
index 39b493f..b3396e3 100644
--- a/Source/cmLocalGenerator.h
+++ b/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,
-                     const char* lang);
+  void AppendDefines(std::set<std::string>& defines,
+                     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,
diff --git a/Source/cmLocalVisualStudio6Generator.cxx b/Source/cmLocalVisualStudio6Generator.cxx
index 1a7e611..9f2a863 100644
--- a/Source/cmLocalVisualStudio6Generator.cxx
+++ b/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::string defines = " ";
-    std::string debugDefines = " ";
-    std::string releaseDefines = " ";
-    std::string minsizeDefines = " ";
-    std::string debugrelDefines = " ";
+    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(
-      defines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS"), 0);
+      definesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS"));
     this->AppendDefines(
-      debugDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_DEBUG"),0);
+      debugDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_DEBUG"));
     this->AppendDefines(
-      releaseDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELEASE"), 0);
+      releaseDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELEASE"));
     this->AppendDefines(
-      minsizeDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"), 0);
+      minsizeDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"));
     this->AppendDefines(
-      debugrelDefines,
-      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"), 0);
+      debugrelDefinesSet,
+      this->Makefile->GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"));
 
     this->AppendDefines(
-      defines,
-      target.GetProperty("COMPILE_DEFINITIONS"), 0);
+      definesSet,
+      target.GetProperty("COMPILE_DEFINITIONS"));
     this->AppendDefines(
-      debugDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_DEBUG"), 0);
+      debugDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_DEBUG"));
     this->AppendDefines(
-      releaseDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_RELEASE"), 0);
+      releaseDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_RELEASE"));
     this->AppendDefines(
-      minsizeDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"), 0);
+      minsizeDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_MINSIZEREL"));
     this->AppendDefines(
-      debugrelDefines,
-      target.GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"), 0);
+      debugrelDefinesSet,
+      target.GetProperty("COMPILE_DEFINITIONS_RELWITHDEBINFO"));
+
+    std::string defines = " ";
+    std::string debugDefines = " ";
+    std::string releaseDefines = " ";
+    std::string minsizeDefines = " ";
+    std::string debugrelDefines = " ";
+
+    this->JoinDefines(definesSet, defines, 0);
+    this->JoinDefines(debugDefinesSet, debugDefines, 0);
+    this->JoinDefines(releaseDefinesSet, releaseDefines, 0);
+    this->JoinDefines(minsizeDefinesSet, minsizeDefines, 0);
+    this->JoinDefines(debugrelDefinesSet, debugrelDefines, 0);
+
     flags += defines;
     flagsDebug += debugDefines;
     flagsRelease += releaseDefines;
diff --git a/Source/cmMakefileTargetGenerator.cxx b/Source/cmMakefileTargetGenerator.cxx
index 0de182e..95738c4 100644
--- a/Source/cmMakefileTargetGenerator.cxx
+++ b/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 = "$(";
-  defines += lang;
-  defines += "_DEFINES)";
+  std::set<std::string> 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));
diff --git a/Source/cmNinjaTargetGenerator.cxx b/Source/cmNinjaTargetGenerator.cxx
index 3532c8b..185965c 100644
--- a/Source/cmNinjaTargetGenerator.cxx
+++ b/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,
-                                        language.c_str());
+    this->LocalGenerator->AppendDefines(defines, exportMacro);
     }
 
   // Add preprocessor definitions for this target and configuration.
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Makefile->GetProperty("COMPILE_DEFINITIONS"),
-     language.c_str());
+     this->Makefile->GetProperty("COMPILE_DEFINITIONS"));
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Target->GetProperty("COMPILE_DEFINITIONS"),
-     language.c_str());
+     this->Target->GetProperty("COMPILE_DEFINITIONS"));
   this->LocalGenerator->AppendDefines
     (defines,
-     source->GetProperty("COMPILE_DEFINITIONS"),
-     language.c_str());
+     source->GetProperty("COMPILE_DEFINITIONS"));
   {
   std::string defPropName = "COMPILE_DEFINITIONS_";
   defPropName += cmSystemTools::UpperCase(this->GetConfigName());
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Makefile->GetProperty(defPropName.c_str()),
-     language.c_str());
+     this->Makefile->GetProperty(defPropName.c_str()));
   this->LocalGenerator->AppendDefines
     (defines,
-     this->Target->GetProperty(defPropName.c_str()),
-     language.c_str());
+     this->Target->GetProperty(defPropName.c_str()));
   this->LocalGenerator->AppendDefines
     (defines,
-     source->GetProperty(defPropName.c_str()),
-     language.c_str());
+     source->GetProperty(defPropName.c_str()));
   }
 
-  return defines;
+  std::string definesString;
+  this->LocalGenerator->JoinDefines(defines, definesString,
+     language.c_str());
+
+  return definesString;
 }
 
 cmNinjaDeps cmNinjaTargetGenerator::ComputeLinkDeps() const
diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt
index 9512ea6..2397577 100644
--- a/Tests/CMakeLists.txt
+++ b/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)
diff --git a/Tests/CompileDefinitions/CMakeLists.txt b/Tests/CompileDefinitions/CMakeLists.txt
new file mode 100644
index 0000000..3e4181b
--- /dev/null
+++ b/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")
diff --git a/Tests/CompileDefinitions/add_definitions_command/CMakeLists.txt b/Tests/CompileDefinitions/add_definitions_command/CMakeLists.txt
new file mode 100644
index 0000000..4ca269d
--- /dev/null
+++ b/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)
diff --git a/Tests/CompileDefinitions/add_definitions_command_with_target_prop/CMakeLists.txt b/Tests/CompileDefinitions/add_definitions_command_with_target_prop/CMakeLists.txt
new file mode 100644
index 0000000..4161ed6
--- /dev/null
+++ b/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")
diff --git a/Tests/CompileDefinitions/main.cpp b/Tests/CompileDefinitions/main.cpp
new file mode 100644
index 0000000..d80c9dc
--- /dev/null
+++ b/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;
+}
diff --git a/Tests/CompileDefinitions/target_prop/CMakeLists.txt b/Tests/CompileDefinitions/target_prop/CMakeLists.txt
new file mode 100644
index 0000000..8248a21
--- /dev/null
+++ b/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")