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 >>http://en.wikipedia.org/wiki/Syslog#Severity_levels >> > 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 http://openvswitch.org/mailman/listinfo/dev
