Define GOOGLE_ATTRIBUTE_NOINLINE for MSVC. Workaround for VS2015 Release build compiler bug.

See issue #240 - MSVC in VS2015 seems to inline a function it shouldn't. My original workaround was to disable inlining for the whole file, but I found a way to do it on just this specific function using __declspec(noinline).
Unfortunately __declspec has to go at the start of the function declaration, while __attribute in GCC can go either before or after. I had to move lots of GOOGLE_ATTRIBUTE_NOLINE to make it compile. I have not yet tested this change with GCC.

Will there be other side effects of defining this, given it wasn't previously?

I also noticed a few functions marked with both the 'inline' keyword, and GOOGLE_ATTRIBUTE_NOINLINE - huh? Is there an explanation for this, or is it an oversight?

6 files changed, 31 insertions, 23 deletions

diff --git a/src/google/protobuf/arena.h b/src/google/protobuf/arena.h
index 51149bae..e7e693b2 100644
--- a/src/google/protobuf/arena.h
+++ b/src/google/protobuf/arena.h
@@ -425,16 +425,16 @@ class LIBPROTOBUF_EXPORT Arena {
   // of the underlying blocks. The total space used may not include the new
   // blocks that are allocated by this arena from other threads concurrently
   // with the call to this method.
-  uint64 SpaceAllocated() const GOOGLE_ATTRIBUTE_NOINLINE;
+  GOOGLE_ATTRIBUTE_NOINLINE uint64 SpaceAllocated() const;
   // As above, but does not include any free space in underlying blocks.
-  uint64 SpaceUsed() const GOOGLE_ATTRIBUTE_NOINLINE;
+  GOOGLE_ATTRIBUTE_NOINLINE uint64 SpaceUsed() const;
 
   // Frees all storage allocated by this arena after calling destructors
   // registered with OwnDestructor() and freeing objects registered with Own().
   // Any objects allocated on this arena are unusable after this call. It also
   // returns the total space used by the arena which is the sums of the sizes
   // of the allocated blocks. This method is not thread-safe.
-  uint64 Reset() GOOGLE_ATTRIBUTE_NOINLINE;
+  GOOGLE_ATTRIBUTE_NOINLINE uint64 Reset();
 
   // Adds |object| to a list of heap-allocated objects to be freed with |delete|
   // when the arena is destroyed or reset.
@@ -459,8 +459,7 @@ class LIBPROTOBUF_EXPORT Arena {
   // will be manually called when the arena is destroyed or reset. This differs
   // from OwnDestructor() in that any member function may be specified, not only
   // the class destructor.
-  void OwnCustomDestructor(void* object, void (*destruct)(void*))
-      GOOGLE_ATTRIBUTE_NOINLINE {
+  GOOGLE_ATTRIBUTE_NOINLINE void OwnCustomDestructor(void* object, void (*destruct)(void*)) {
     AddListNode(object, destruct);
   }
 
diff --git a/src/google/protobuf/arenastring.h b/src/google/protobuf/arenastring.h
index d829ed91..7bbf6c76 100755
--- a/src/google/protobuf/arenastring.h
+++ b/src/google/protobuf/arenastring.h
@@ -283,9 +283,9 @@ struct LIBPROTOBUF_EXPORT ArenaStringPtr {
  private:
   ::std::string* ptr_;
 
+  GOOGLE_ATTRIBUTE_NOINLINE
   inline void CreateInstance(::google::protobuf::Arena* arena,
-                             const ::std::string* initial_value)
-      GOOGLE_ATTRIBUTE_NOINLINE {
+                             const ::std::string* initial_value) {
     // Assumes ptr_ is not NULL.
     if (initial_value != NULL) {
       ptr_ = new ::std::string(*initial_value);
@@ -296,8 +296,9 @@ struct LIBPROTOBUF_EXPORT ArenaStringPtr {
       arena->Own(ptr_);
     }
   }
-  inline void CreateInstanceNoArena(const ::std::string* initial_value)
-      GOOGLE_ATTRIBUTE_NOINLINE {
+
+  GOOGLE_ATTRIBUTE_NOINLINE
+  inline void CreateInstanceNoArena(const ::std::string* initial_value) {
     if (initial_value != NULL) {
       ptr_ = new ::std::string(*initial_value);
     } else {
diff --git a/src/google/protobuf/message.cc b/src/google/protobuf/message.cc
index 276d7de5..123bd789 100644
--- a/src/google/protobuf/message.cc
+++ b/src/google/protobuf/message.cc
@@ -52,6 +52,7 @@
 #include <google/protobuf/stubs/map_util.h>
 #include <google/protobuf/stubs/singleton.h>
 #include <google/protobuf/stubs/stl_util.h>
+#include <google/protobuf/stubs/port.h>
 
 namespace google {
 namespace protobuf {
@@ -466,6 +467,10 @@ struct ShutdownRepeatedFieldRegister {
 
 namespace internal {
 template<>
+#if defined(_MSC_VER) && (_MSC_VER >= 1900)
+// Note: force noinline to workaround MSVC 2015 compiler bug, issue #240
+GOOGLE_ATTRIBUTE_NOINLINE
+#endif
 Message* GenericTypeHandler<Message>::NewFromPrototype(
     const Message* prototype, google::protobuf::Arena* arena) {
   return prototype->New(arena);
diff --git a/src/google/protobuf/metadata.h b/src/google/protobuf/metadata.h
index 30b2a6ee..c5bab0a8 100644
--- a/src/google/protobuf/metadata.h
+++ b/src/google/protobuf/metadata.h
@@ -143,7 +143,7 @@ class LIBPROTOBUF_EXPORT InternalMetadataWithArena {
     Arena* arena_;
   };
 
-  UnknownFieldSet* mutable_unknown_fields_slow() GOOGLE_ATTRIBUTE_NOINLINE {
+  GOOGLE_ATTRIBUTE_NOINLINE UnknownFieldSet* mutable_unknown_fields_slow() {
     Arena* my_arena = arena();
     Container* container = Arena::Create<Container>(my_arena);
     ptr_ = reinterpret_cast<void*>(
diff --git a/src/google/protobuf/repeated_field.h b/src/google/protobuf/repeated_field.h
index 14f46298..9c84ed98 100644
--- a/src/google/protobuf/repeated_field.h
+++ b/src/google/protobuf/repeated_field.h
@@ -458,22 +458,21 @@ class LIBPROTOBUF_EXPORT RepeatedPtrFieldBase {
   void AddAllocatedInternal(typename TypeHandler::Type* value,
                             google::protobuf::internal::false_type);
 
-  template <typename TypeHandler>
+  template <typename TypeHandler> GOOGLE_ATTRIBUTE_NOINLINE
   void AddAllocatedSlowWithCopy(typename TypeHandler::Type* value,
                                 Arena* value_arena,
                                 Arena* my_arena)
-      GOOGLE_ATTRIBUTE_NOINLINE;
-  template <typename TypeHandler>
-  void AddAllocatedSlowWithoutCopy(typename TypeHandler::Type* value)
-      GOOGLE_ATTRIBUTE_NOINLINE;
+;
+  template <typename TypeHandler> GOOGLE_ATTRIBUTE_NOINLINE
+  void AddAllocatedSlowWithoutCopy(typename TypeHandler::Type* value);
 
   template <typename TypeHandler>
   typename TypeHandler::Type* ReleaseLastInternal(google::protobuf::internal::true_type);
   template <typename TypeHandler>
   typename TypeHandler::Type* ReleaseLastInternal(google::protobuf::internal::false_type);
 
-  template<typename TypeHandler>
-  inline void SwapFallback(RepeatedPtrFieldBase* other) GOOGLE_ATTRIBUTE_NOINLINE;
+  template<typename TypeHandler> GOOGLE_ATTRIBUTE_NOINLINE
+  inline void SwapFallback(RepeatedPtrFieldBase* other);
 
   inline Arena* GetArenaNoVirtual() const {
     return arena_;
@@ -545,13 +544,13 @@ class GenericTypeHandler {
   // constructors and destructors. Note that the GOOGLE_ATTRIBUTE_NOINLINE macro
   // requires the 'inline' storage class here, which is somewhat confusing, but
   // the compiler does the right thing.
+  GOOGLE_ATTRIBUTE_NOINLINE
   static inline GenericType* NewFromPrototype(const GenericType* prototype,
-                                              ::google::protobuf::Arena* arena = NULL)
-    GOOGLE_ATTRIBUTE_NOINLINE {
+                                              ::google::protobuf::Arena* arena = NULL) {
     return New(arena);
   }
-  static inline void Delete(GenericType* value, Arena* arena)
-    GOOGLE_ATTRIBUTE_NOINLINE {
+  GOOGLE_ATTRIBUTE_NOINLINE
+  static inline void Delete(GenericType* value, Arena* arena) {
     if (arena == NULL) {
       delete value;
     }
@@ -564,8 +563,9 @@ class GenericTypeHandler {
   }
 
   static inline void Clear(GenericType* value) { value->Clear(); }
-  static inline void Merge(const GenericType& from, GenericType* to)
-      GOOGLE_ATTRIBUTE_NOINLINE {
+
+  GOOGLE_ATTRIBUTE_NOINLINE
+  static inline void Merge(const GenericType& from, GenericType* to) {
     to->MergeFrom(from);
   }
   static inline int SpaceUsed(const GenericType& value) {
diff --git a/src/google/protobuf/stubs/port.h b/src/google/protobuf/stubs/port.h
index 8a5d1a13..a3c53dd9 100644
--- a/src/google/protobuf/stubs/port.h
+++ b/src/google/protobuf/stubs/port.h
@@ -161,6 +161,9 @@ static const uint64 kuint64max = GOOGLE_ULONGLONG(0xFFFFFFFFFFFFFFFF);
 // For functions we want to force not inline.
 // Introduced in gcc 3.1.
 #define GOOGLE_ATTRIBUTE_NOINLINE __attribute__ ((noinline))
+#elif defined(_MSC_VER) && (_MSC_VER >= 1400)
+// Seems to have been around since at least Visual Studio 2005
+#define GOOGLE_ATTRIBUTE_NOINLINE __declspec(noinline)
 #else
 // Other compilers will have to figure it out for themselves.
 #define GOOGLE_ATTRIBUTE_NOINLINE