[llvm] r234343 - Enable W4 warnings by default for MSVC builds

[Aaron Ballman]

On Thu, Apr 16, 2015 at 11:43 AM, Yaron Keren <yaron.keren at gmail.com> wrote:
> This LGTM, with #ifdef _MSC_VER macro of course, maybe in Compiler.h ?

That was what I was thinking, yes.

> I wouldn't mind disabling the warning completely either, I don't find it
> helpful.

Since this is the only instance of that warning, and we may want to
fix it to keep the published support headers compiling at /W4 cleanly,
I don't see a reason to disable the warning. This warning *could* give
us a heads up on behavioral differences for binary formats due to
__declspec(align), but it's just as likely to trigger false positives,
so I wouldn't argue too hard against disabling the warning, either.

~Aaron

>
>
> 2015-04-16 18:12 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>>
>> Since AlignOf is part of our support headers, I would slightly prefer
>> if we could fix it there. As an experiment, I put in the proper
>> #pragma incantation around the declaration of AlignmentCalcImpl<T> in
>> AlignOf.h, and it resolved *all* of the warnings.
>>
>> #pragma warning(push)
>> #pragma warning(disable : 4324)
>> template <typename T>
>> struct AlignmentCalcImpl {
>>   char x;
>>   T t;
>> private:
>>   AlignmentCalcImpl() {} // Never instantiate.
>> };
>> #pragma warning(pop)
>>
>>
>> While the #pragma dance is ugly (and would need to be abstracted away
>> via macros), it may be worth it since this is a support header (and we
>> cannot fix the padding issue because that's the whole point to that
>> structure). How do others feel?
>>
>> ~Aaron
>>
>> On Thu, Apr 16, 2015 at 10:49 AM, Yaron Keren <yaron.keren at gmail.com>
>> wrote:
>> > /W3 was probably cached from the previous build.
>> >
>> > So should we disable C4324 or fix (how?) the AlignOf?
>> >
>> >
>> > 2015-04-16 17:34 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>> >>
>> >> It seems that in order to trigger this, you have to have done a clean
>> >> cmake build, as I am now seeing the same warnings you are.
>> >>
>> >> ~Aaron
>> >>
>> >> On Thu, Apr 16, 2015 at 10:16 AM, Aaron Ballman
>> >> <aaron at aaronballman.com>
>> >> wrote:
>> >> > On Thu, Apr 16, 2015 at 10:03 AM, Yaron Keren <yaron.keren at gmail.com>
>> >> > wrote:
>> >> >> It is same in the IDE and command line, Release or Debug
>> >> >> (unthreaded),
>> >> >> 32
>> >> >> bit.
>> >> >> The warning is:
>> >> >>
>> >> >>   PPMacroExpansion.cpp
>> >> >> c:\llvm\include\llvm/Support/AlignOf.h(25): warning C4324:
>> >> >> 'llvm::AlignmentCalcImpl<T>' : structure was padded due to
>> >> >> __declspec(align())
>> >> >> [c:\llvm\msvc\tools\clang\lib\Lex\clangLex.vcxproj]
>> >> >>           with
>> >> >>           [
>> >> >>               T=clang::SrcMgr::ContentCache
>> >> >>           ] (c:\llvm\tools\clang\lib\Lex\PPCaching.cpp)
>> >> >>           c:\llvm\include\llvm/Support/AlignOf.h(40) : see reference
>> >> >> to
>> >> >> class template instantiation 'llvm::AlignmentC
>> >> >>   alcImpl<T>' being compiled
>> >> >>           with
>> >> >>           [
>> >> >>               T=clang::SrcMgr::ContentCache
>> >> >>           ]
>> >> >>
>> >> >> c:\llvm\tools\clang\include\clang/Basic/SourceManager.h(225)
>> >> >> : see
>> >> >> reference to class template instantiation
>> >> >>    'llvm::AlignOf<clang::SrcMgr::ContentCache>' being compiled
>> >> >>
>> >> >> referring to SourceManager.h(225):
>> >> >>
>> >> >>   static_assert(llvm::AlignOf<ContentCache>::Alignment >= 8,
>> >> >>                 "ContentCache must be 8-byte aligned.");
>> >> >>
>> >> >> And appears whenever SourceManager.h is included, a lot.
>> >> >>
>> >> >> The compiler is Microsoft Visual Studio Ultimate 2013,  Version
>> >> >> 12.0.31101.00 Update 4.
>> >> >>
>> >> >> I'll try threaded build now.
>> >> >
>> >> > I will do a clean rebuild from scratch; I wonder if the reason I'm
>> >> > not
>> >> > seeing anything is because of some caching somewhere. Thank you!
>> >> >
>> >> > ~Aaron
>> >
>> >
>
>