Fix some warnings on MSVC

[aganea]

Summary

Fixes the following warnings on (latest) MSVC:

1. In Kaleidoscope-Ch2: "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll"

2. In gtest (see this link):

f:\svn\llvm\utils\unittest\googlemock\include\gmock\gmock-matchers.h(183): warning C5046: 'testing::MatcherInterface<T>::~MatcherInterface': Symbol involving type with internal linkage not defined
23>        with
23>        [
23>            T=`anonymous-namespace'::CustomSubError &
23>        ] (compiling source file F:\svn\llvm\unittests\Support\ErrorTest.cpp)

3. In SupportTests: cl : Command line warning D9025: overriding '/W4' with '/w'

[gkistanova]

Thanks for working on this, Alexandre!

1. ignore "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll

Doesn't this warning indicate that Kaleidoscope-Ch2 is not actually calling any functions from the shell32.dll it is trying to delay-load.
Fixing it instead of disabling the warning might be a better move, what do you think?

The rest looks good to me.

[aganea]

In D56329#1348784, @gkistanova wrote:

1. ignore "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll

Doesn't this warning indicate that Kaleidoscope-Ch2 is not actually calling any functions from the shell32.dll it is trying to delay-load.
Fixing it instead of disabling the warning might be a better move, what do you think?

I've tried several things and I can't find a good way to do that. Would you have anything to suggest?
-delayload:shell32.dll is added in /llvm/trunk/lib/Support/CMakeLists.txt. I don't see how to disable that from within /llvm/trunk/examples/Kaleidoscope/Chapter2/CMakeLists.txt?

[rnk]

Non gmock changes look good.

utils/unittest/googlemock/include/gmock/gmock-matchers.h

145–148

C5046 seems like some kind of undocumented warning:
https://developercommunity.visualstudio.com/content/problem/288509/undocumented-warning-c5046-symbol-involving-type-w.html

Maybe it will go away in new versions of VS if we do nothing? I don't see anything wrong with this code. It has an implicit virtual destructor because its base class has one.

I guess I'd like to hear more about how this comes up in practice before applying this change, especially since it's imported gmock code.

[aganea]

utils/unittest/googlemock/include/gmock/gmock-matchers.h

145–148

Yeah it's a warning that came up in the last months, since one of the VS2017 15.8.XX upgrades. The warning I'm seeing is this:

46>f:\svn\llvm\utils\unittest\googlemock\include\gmock\gmock-matchers.h(186): warning C5046: 'testing::MatcherInterface<T>::~MatcherInterface': Symbol involving type with internal linkage not defined
46>        with
46>        [
46>            T=`anonymous-namespace'::CustomSubError &
46>        ] (compiling source file F:\svn\llvm\unittests\Support\ErrorTest.cpp)

You're probably right, it's most likely a spurious warning. For a similar example, your linked webpage says:

Jonathan Emmett [MSFT]
Thanks for this example. This is indeed a false-positive and we have a fix for this particular issue related to special member functions. It will be addressed in Visual Studio 2019.

Maybe add -ignore:5046 for ErrorTest.cpp in the meanwhile?

[rnk]

utils/unittest/googlemock/include/gmock/gmock-matchers.h

145–148

Sounds reasonable.

[aganea]

unittests/Support/CMakeLists.txt

93

@rnk: Regarding your last comment, please see my latest updated diff (lines 92-100 below). This fixes the warning.

[rnk]

lgtm

[chrisjackson]

Hi @aganea ,

With regards to

93 if( CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 19.15 )

I believe

VERSION_GREATER_EQUAL

requires CMake version 3.7, see the CMake 3.7 release notes.

The LLVM documents and llvm/CmakeLists.txt list the minimum required CMake version for LLVM as 3.4.3. I believe that the minimum CMake version should be updated or an alternative method for checking the compiler version should be used.

[aganea]

Hi Chris, indeed this was fixed in rL352378

examples/Kaleidoscope/Chapter2/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_kaleidoscope_chapter(Kaleidoscope-Ch2
   toy.cpp
   )
 
 if(LLVM_COMPILER_IS_GCC_COMPATIBLE)
   target_compile_options(Kaleidoscope-Ch2 PRIVATE
     -Wno-unused-private-field
     )
 endif()
+
+if(MSVC)
+  # ignore "warning LNK4199: /DELAYLOAD:shell32.dll ignored; no imports found from shell32.dll"
+  target_link_libraries(Kaleidoscope-Ch2 PRIVATE "-ignore:4199")
+endif()

unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   xxhashTest.cpp
   )
 
 target_link_libraries(SupportTests PRIVATE LLVMTestingSupport)
 
 # Disable all warning for AlignOfTest.cpp,
 # as it does things intentionally, and there is no reliable way of
 # disabling all warnings for all the compilers by using pragmas.
+# Don't disable on MSVC, because all incriminated warnings are already disabled
+# in source; and because we would otherwise see this warning:
+#   cl : Command line warning D9025: overriding '/W4' with '/w'
+if(NOT MSVC)
   set_source_files_properties(AlignOfTest.cpp PROPERTIES COMPILE_FLAGS -w)
+endif()
 
 # ManagedStatic.cpp uses <pthread>.
 target_link_libraries(SupportTests PRIVATE LLVMTestingSupport ${LLVM_PTHREAD_LIB})
 
 add_subdirectory(DynamicLibrary)

utils/unittest/googlemock/include/gmock/gmock-matchers.h

virtual void DescribeNegationTo(::std::ostream* os) const {
     *os << ")";
   }
 };
 
 // The implementation of a matcher.
 template <typename T>
 class MatcherInterface : public MatcherDescriberInterface {
  public:
+  // Define virtual destructor to avoid the following MSVC warning:
+  // warning C5046: 'testing::MatcherInterface<T>::~MatcherInterface':
+  //    Symbol involving type with internal linkage not defined
+  virtual ~MatcherInterface() {}
+
   // Returns true iff the matcher matches x; also explains the match
   // result to 'listener' if necessary (see the next paragraph), in
   // the form of a non-restrictive relative clause ("which ...",
   // "whose ...", etc) that describes x.  For example, the
   // MatchAndExplain() method of the Pointee(...) matcher should
   // generate an explanation like "which points to ...".
   //
   // Implementations of MatchAndExplain() should add an explanation of