On Mon, Dec 16, 2013 at 11:39 AM, Eitan Eliahu <elia...@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" <shet...@nicira.com> > To: "Alin Serdean" <aserd...@cloudbasesolutions.com> > Cc: dev@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 > <aserd...@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@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
