You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/02/04 20:36:09 UTC
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
pdxcodemonkey opened a new pull request #741:
URL: https://github.com/apache/geode-native/pull/741
- This would bite developers from time to time, because code with extra
qualifications will build on Windows, but all other platforms will
flag it with a warning and break the build.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #741:
URL: https://github.com/apache/geode-native/pull/741#discussion_r571219081
##########
File path: cppcache/integration/benchmark/RegionBM.cpp
##########
@@ -19,6 +19,17 @@
#include <framework/Cluster.h>
#include <framework/Gfsh.h>
+// Disable warning for "extra qualifications" here. One of the boost log
+// headers triggers this warning. Note: use of disable pragma here is
+// intentional - attempts to use push/pop as you ordinarily should just
+// yielded a gripe from the MS tools that "warning number '4596' is not a
+// valid compiler warning". re-enabling the warning after the include
+// fails in the same way, so just leave it disabled for the rest of the
+// file. This is safe, since the warning can only trigger inside a class
Review comment:
What I'm saying is there's _no_ class defined in this file, cause that's a thing typically done in headers. Thus, leaving the warning disabled for the rest of the .cpp file has no effect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #741:
URL: https://github.com/apache/geode-native/pull/741
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #741:
URL: https://github.com/apache/geode-native/pull/741#discussion_r571166567
##########
File path: CMakeLists.txt
##########
@@ -281,6 +281,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
target_compile_options(_WarningsAsError INTERFACE
/WX
/wd4996
+ /we4596
Review comment:
🙄 🤦
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #741:
URL: https://github.com/apache/geode-native/pull/741#discussion_r571142016
##########
File path: CMakeLists.txt
##########
@@ -281,6 +281,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "MSVC")
target_compile_options(_WarningsAsError INTERFACE
/WX
/wd4996
+ /we4596
Review comment:
I was going to write indentation, but I actually am coming around to indenting every line with different indentation
##########
File path: cppcache/integration/benchmark/RegionBM.cpp
##########
@@ -19,6 +19,17 @@
#include <framework/Cluster.h>
#include <framework/Gfsh.h>
+// Disable warning for "extra qualifications" here. One of the boost log
+// headers triggers this warning. Note: use of disable pragma here is
+// intentional - attempts to use push/pop as you ordinarily should just
+// yielded a gripe from the MS tools that "warning number '4596' is not a
+// valid compiler warning". re-enabling the warning after the include
+// fails in the same way, so just leave it disabled for the rest of the
+// file. This is safe, since the warning can only trigger inside a class
Review comment:
There's a class defined on line 51? Or did you mean something else by this comment?
##########
File path: clicache/src/impl/PdxHelper.hpp
##########
@@ -46,15 +46,15 @@ namespace Apache
static IPdxSerializable^ DeserializePdx(DataInput^ dataOutput, bool forceDeserialize, const native::SerializationRegistry* serializationRegistry);
- static IPdxSerializable^ PdxHelper::DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
+ static IPdxSerializable^ DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
literal Byte PdxHeader = 8;
static Int32 ReadInt32(System::Byte* offsetPosition);
static Int32 ReadInt16(System::Byte* offsetPosition);
- static Int32 PdxHelper::ReadUInt16(System::Byte* offsetPosition);
+ static Int32 ReadUInt16(System::Byte* offsetPosition);
Review comment:
Still weird indentation if you decide it's worth it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #741:
URL: https://github.com/apache/geode-native/pull/741#discussion_r571221044
##########
File path: clicache/src/impl/PdxHelper.hpp
##########
@@ -46,15 +46,15 @@ namespace Apache
static IPdxSerializable^ DeserializePdx(DataInput^ dataOutput, bool forceDeserialize, const native::SerializationRegistry* serializationRegistry);
- static IPdxSerializable^ PdxHelper::DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
+ static IPdxSerializable^ DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
literal Byte PdxHeader = 8;
static Int32 ReadInt32(System::Byte* offsetPosition);
static Int32 ReadInt16(System::Byte* offsetPosition);
- static Int32 PdxHelper::ReadUInt16(System::Byte* offsetPosition);
+ static Int32 ReadUInt16(System::Byte* offsetPosition);
Review comment:
Meh - tabs. Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #741: GEODE-8921: Enable "extra qualification" warning in Windows builds
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #741:
URL: https://github.com/apache/geode-native/pull/741#discussion_r571166040
##########
File path: clicache/src/impl/PdxHelper.hpp
##########
@@ -46,15 +46,15 @@ namespace Apache
static IPdxSerializable^ DeserializePdx(DataInput^ dataOutput, bool forceDeserialize, const native::SerializationRegistry* serializationRegistry);
- static IPdxSerializable^ PdxHelper::DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
+ static IPdxSerializable^ DeserializePdx(DataInput^ dataInput, bool forceDeserialize, int typeId, int length, const native::SerializationRegistry* serializationRegistry);
literal Byte PdxHeader = 8;
static Int32 ReadInt32(System::Byte* offsetPosition);
static Int32 ReadInt16(System::Byte* offsetPosition);
- static Int32 PdxHelper::ReadUInt16(System::Byte* offsetPosition);
+ static Int32 ReadUInt16(System::Byte* offsetPosition);
Review comment:
please fix this - its a pain that just continues if we don't keep it tidy...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org