[ovs-dev] [PATCH] V2 windefs: common include for MSVC
Gurucharan Shetty
shettyg at nicira.com
Mon Dec 16 20:56:40 UTC 2013
On Mon, Dec 16, 2013 at 11:39 AM, Eitan Eliahu <eliahue at vmware.com> wrote:
>
> Could we use #pragma warning(push) and pop to reduce the scope of the warning suppression.
Yeah. That is probably something we can use in cases where we cannot
fix the warnings nicely.
> Thanks,
> Eitan
>
> ----- Original Message -----
> From: "Gurucharan Shetty" <shettyg at nicira.com>
> To: "Alin Serdean" <aserdean at cloudbasesolutions.com>
> Cc: dev at openvswitch.org
> Sent: Monday, December 16, 2013 11:18:28 AM
> Subject: Re: [ovs-dev] [PATCH] V2 windefs: common include for MSVC
>
> On Mon, Dec 16, 2013 at 10:18 AM, Alin Serdean
> <aserdean at cloudbasesolutions.com> wrote:
>>>> +//Disable warnings and runtime checks
>>>> +#pragma warning( disable : 4116 4090 4700 4005 4133 4028 4098 4293 4715 4047)
>>>> +#pragma runtime_checks( "", off )
>>>> +#pragma warning( disable : 4996 ) //deprecated functions
>>>> +#pragma warning( disable : 4244 ) //possible loss of data
>>>> +#pragma warning( disable : 4146 ) //unary minus operator applied to unsigned type, result still unsigned
>>>> +#pragma warning( disable : 4018 ) //'>=' : signed/unsigned mismatch
>>>
>>>Why are we disabling the warnings? Some of them look like may actually
>>>help with bugs. Worst case of not disabling them is that we will seem
>>>them during compilation right?
>>>
>> Majority of them are the same as the onex being passed in gcc (-Wno-sign-compare -Wunused-parameter etc. ) but I have no problem removing them it will just be a longer log to look at :).
>
> I see. Instead of starting with a large list, how about we start with
> a smaller list and then add things to it in the future as we see
> issues?
> The 2 warnings that are disabled while using gcc are -Wno-sign-compare
> and -Wno-format-zero-length. Can you add the corresponding error
> numbers with a descriptive comment of the form /* */ in a separate
> line and not "//" (You can look at CodingStyle for the accepted way of
> commenting).
>
> I would have preferred to add the conversion of warnings in
> build-aux/cccl in the case statement, but it is possible that there is
> no 1-1 mapping between gcc and cl warnings.
>
>> Although this pragma: #pragma runtime_checks( "", off ) if not there will be runtime checks if there are local variables used but not set.
> Is it not disabled by default and only enabled with /RTC as a
> compilation option? (I could be wrong).
>
>>>
>>>> +#pragma comment(lib, "dbghelp.lib")
>>>> +#pragma comment(lib, "Advapi32.lib")
>>>> +#pragma comment (lib, "MsWSock.Lib")
>>>> +#pragma comment (lib, "WS2_32.Lib")
>>>
>>>Should we only add the above in a commit that includes code changes
>>>that needs any of these libraries. Otherwise, when any of the
>>>functions that use these libraries are actually removed in the future,
>>>we won't know to remove these libraries. For example, I do not know
>>>why we need the dbghelp.lib. But, if you add it to this file at the
>>>same time you make a change in code which needs this library, I will
>>>know.
>>>
>> Agreed. I will remove them when I send the new patch.
>>>I looked at doing this with autoconf's AC_SEARCH_LIBS, but it doesn't
>>>work for windows. We can do this with AC_LINK_IFELSE(), but I am not
>>>sure if it has any extra advantages compared to #pragma comment.
>>>
>>>> +#define caddr_t char*
>>>I see that caddr_t is only used in lib/netdev-linux.c. If we don't
>>>compile that file for Windows, we won't need the above #define.
>> I will remove it in the new patch.
>>>> +#define ssize_t int
>>>I see some recommendations of using SSIZE_T from BaseTsd.h. Would that
>>>make sense?
>> Totally makes sense and will change it in the new patch.
>>>
>>>> +#define LOG_EMERG 1
>>>> +#define LOG_ALERT 1
>>>> +#define LOG_CRIT 1
>>>> +#define LOG_ERR 4
>>>> +#define LOG_WARNING 5
>>>> +#define LOG_NOTICE 6
>>>> +#define LOG_INFO 6
>>>> +#define LOG_DEBUG 6
>>>> +#define LOG_NDELAY 6
>>>> +#define LOG_DAEMON 6
>>>
>>>Is there any reason for redefinition of the above a little differently
>>>than what we have for Linux? Why not use the
>>>https://urldefense.proofpoint.com/v1/url?u=http://en.wikipedia.org/wiki/Syslog%23Severity_levels&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=rZQDadlYD9v3nRf%2BUKWnRSFvWx6p5zP%2BgFBoRgm%2B8DU%3D%0A&s=3428868b698daf6b358ce777a6d5c6eb05c9d74d6c12926e7781a04d4ef84b79
>>>
>> You are totally right. What do you think of the following:
>> #define LOG_EMERG 0 /* system is unusable */
>> #define LOG_ALERT 1 /* action must be taken immediately */
>> #define LOG_CRIT 2 /* critical conditions */
>> #define LOG_ERR 3 /* error conditions */
>> #define LOG_WARNING 4 /* warning conditions */
>> #define LOG_NOTICE 5 /* normal but significant condition */
>> #define LOG_INFO 6 /* informational */
>> #define LOG_DEBUG 7 /* debug-level messages */
>> #define LOG_NDELAY 8 /* don't delay open */
>> #define LOG_DAEMON 24 /* system daemons */
>
> Okay. Please format it correctly when you add it. Instead of defining
> the above in windefs.h and making this file huge, I think we can
> create a new file called syslog.h and add it to include/windows. Since
> we -I include/windows, the #include <syslog.h> in the code base should
> work.
>
>>>> +
>>>> +//Force undefines from config.h
>>>> +#undef HAVE_EXECINFO_H
>>>> +#undef VLOG_MODULE
>>>> +#undef HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC
>>>> +#undef HAVE_MNTENT_H
>>>> +#undef HAVE_SYS_STATVFS_H
>>>> +#undef HAVE_STATVFS
>>>> +#undef __CHECKER__
>>>> +#undef USE_LINKER_SECTIONS
>>>> +#undef DELETE
>>>> +#undef HAVE_BACKTRACE
>>>> +#undef HAVE_MALLOC_HOOKS
>>>I don't think we have to do the config.h undefs. Autoconf should take
>>>care and undef the above automatically.
>>>I also see VLOG_MODULE being undef'd. I see that on windows, autoconf
>>>will generate vlog-modules.def automatically and vlog logic works
>>>correctly. Don't you see that?
>> Sorry about that :), artefacts from my former port we can remove them.
>>
>> If there are no further remarks I will send in the updated version.
>>
>> Alin.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=rZQDadlYD9v3nRf%2BUKWnRSFvWx6p5zP%2BgFBoRgm%2B8DU%3D%0A&s=1467bfb740e884d0e363f98fcab346af2cdd913b4d924001dfbf7813c8e00703
More information about the dev
mailing list