GH-47099: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47114

behrisch · 2025-07-15T15:55:44Z

Rationale for this change

The missing pragma is causing warnings downstream see #47099

What changes are included in this PR?

Adding the missing pragma warning(pop)

Are these changes tested?

I hope for the CI to do that ;-)

Are there any user-facing changes?

The developer should see less warnings otherwise, no.

GitHub Issue: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47099

github-actions · 2025-07-15T15:56:08Z

⚠️ GitHub issue #47099 has been automatically assigned in GitHub to PR creator.

wgtmac · 2025-07-16T02:05:42Z

Could you try to add following code to fix https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52408405

diff --git a/cpp/src/parquet/exception.h b/cpp/src/parquet/exception.h
index 46f5595dd1..55b769c978 100644
--- a/cpp/src/parquet/exception.h
+++ b/cpp/src/parquet/exception.h
@@ -84,7 +84,7 @@
 
 namespace parquet {
 
-class ParquetException : public std::exception {
+class PARQUET_EXPORT ParquetException : public std::exception {
  public:
   PARQUET_NORETURN static void EofException(const std::string& msg = "") {
     static std::string prefix = "Unexpected end of stream";

behrisch · 2025-07-19T09:32:41Z

Could you try to add following code to fix https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52408405

I did that but it did not help much AFAICS. How do we continue? Should I add another warning push/pop to exception.h?

wgtmac · 2025-07-21T03:10:15Z

FAILED: [code=2] src/parquet/CMakeFiles/parquet_shared.dir/arrow/path_internal.cc.obj 
"C:\Program Files\Git\usr\bin\ccache.exe" C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\cl.exe  /nologo /TP -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_WITH_TIMING_TESTS -DHAVE_INTTYPES_H -DHAVE_NETDB_H -DNOMINMAX -DPARQUET_EXPORTING -DPARQUET_THRIFT_VERSION_MAJOR=0 -DPARQUET_THRIFT_VERSION_MINOR=20 -D_CRT_SECURE_NO_WARNINGS -D__SSE2__ -D__SSE4_1__ -D__SSE4_2__ -Dparquet_shared_EXPORTS -ID:\a\arrow\arrow\build\cpp\src -ID:\a\arrow\arrow\cpp\src -ID:\a\arrow\arrow\cpp\src\generated -external:ID:\a\arrow\arrow\build\cpp\vcpkg_installed\x64-windows\include -external:W0 /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING /W3 /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4365 /wd4267 /wd4838 /wd4800 /wd4996 /wd4065 /arch:SSE4.2  /Ob0 /Od /RTC1 /WX -std:c++17 -MDd -Zi /showIncludes /Fosrc\parquet\CMakeFiles\parquet_shared.dir\arrow\path_internal.cc.obj /Fdsrc\parquet\CMakeFiles\parquet_shared.dir\ /FS -c D:\a\arrow\arrow\cpp\src\parquet\arrow\path_internal.cc
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): error C2220: the following warning is treated as an error
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): warning C4275: non dll-interface class 'std::exception' used as base for dll-interface class 'parquet::ParquetException'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\vcruntime_exception.h(49): note: see declaration of 'std::exception'
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): note: see declaration of 'parquet::ParquetException'
[318/403] Building CXX object src\arrow\flight\CMakeFiles\arrow_flight_shared.dir\transport\grpc\protocol_grpc_internal.cc.obj
[319/403] Building CXX object src\parquet\CMakeFiles\parquet_shared.dir\arrow\reader.cc.obj
FAILED: [code=2] src/parquet/CMakeFiles/parquet_shared.dir/arrow/reader.cc.obj 
"C:\Program Files\Git\usr\bin\ccache.exe" C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\cl.exe  /nologo /TP -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_WITH_TIMING_TESTS -DHAVE_INTTYPES_H -DHAVE_NETDB_H -DNOMINMAX -DPARQUET_EXPORTING -DPARQUET_THRIFT_VERSION_MAJOR=0 -DPARQUET_THRIFT_VERSION_MINOR=20 -D_CRT_SECURE_NO_WARNINGS -D__SSE2__ -D__SSE4_1__ -D__SSE4_2__ -Dparquet_shared_EXPORTS -ID:\a\arrow\arrow\build\cpp\src -ID:\a\arrow\arrow\cpp\src -ID:\a\arrow\arrow\cpp\src\generated -external:ID:\a\arrow\arrow\build\cpp\vcpkg_installed\x64-windows\include -external:W0 /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING /W3 /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4365 /wd4267 /wd4838 /wd4800 /wd4996 /wd4065 /arch:SSE4.2  /Ob0 /Od /RTC1 /WX -std:c++17 -MDd -Zi /showIncludes /Fosrc\parquet\CMakeFiles\parquet_shared.dir\arrow\reader.cc.obj /Fdsrc\parquet\CMakeFiles\parquet_shared.dir\ /FS -c D:\a\arrow\arrow\cpp\src\parquet\arrow\reader.cc
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): error C2220: the following warning is treated as an error
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): warning C4275: non dll-interface class 'std::exception' used as base for dll-interface class 'parquet::ParquetException'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\vcruntime_exception.h(49): note: see declaration of 'std::exception'
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): note: see declaration of 'parquet::ParquetException'

From the above log, warning C4275 is fired but it should be disabled already via pragma warning(disable : 4275 4251). I guess the reason is that adding pragma warning(pop) to the parquet/platform.h header file will immediately swipe out all pushed pragmas at the end of this header file and any source file including parquet/platform.h will not see any pushed pragmas any more.

behrisch · 2025-07-24T10:11:50Z

From the above log, warning C4275 is fired but it should be disabled already via pragma warning(disable : 4275 4251). I guess the reason is that adding pragma warning(pop) to the parquet/platform.h header file will immediately swipe out all pushed pragmas at the end of this header file and any source file including parquet/platform.h will not see any pushed pragmas any more.

Yes, that is also my understanding on how the push / pop mechanism works. The warnings are only disabled for everything between the push and the pop. So we either need to

fix the warning or
add another push/pop to exception.h or
add an include for exception.h to platform.h between the push and the pop so that exception.h won't be reread with the warnings being enabled.

I would prefer option 2 but I have only very limited knowledge about the code base.

wgtmac · 2025-07-24T14:36:18Z

Since parquet/platform.h is included by almost all files under parquet subdirectory, I don't think we need to add pop.

Please correct me if I was wrong. cc @kou @raulcd

behrisch · 2025-07-24T15:46:22Z

Since parquet/platform.h is included by almost all files under parquet subdirectory, I don't think we need to add pop.

But if you don't add the pop, the warning is disabled for every file which does #include parquet/platform.h directly or indirectly. So basically every code file using parquet (not only inside the library but also users of the library). That was the whole point of the issue #47099 as far as I am concerned. If you want to disable the warning only for the CI build, you could achieve this with a command line option to the compiler and get rid of the push as well. But every push needs its pop :-)

wgtmac · 2025-07-25T03:13:07Z

As you can see, adding pop to the header file is not sufficient. Since ParquetException is used almost by all Parquet source files, we will end up with adding pop to every source file which seems meaningless if the warning can't be fixed.

kou · 2025-07-25T03:35:01Z

you could achieve this with a command line option to the compiler and get rid of the push as well

Can we use this approach something like the following?

diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt
index dc7d40d2a3..717496496a 100644
--- a/cpp/src/parquet/CMakeLists.txt
+++ b/cpp/src/parquet/CMakeLists.txt
@@ -352,6 +352,18 @@ foreach(LIB_TARGET ${PARQUET_LIBRARIES})
   endif()
 endforeach()
 
+# Disable warnings
+foreach(LIB_TARGET ${PARQUET_LIBRARIES})
+  if(MSVC)
+    # Disable warning for STL types usage in DLL interface
+    # https://web.archive.org/web/20130317015847/http://connect.microsoft.com/VisualStudio/feedback/details/696593/vc-10-vs-2010-basic-string-exports
+    target_compile_options(${LIB_TARGET} PUBLIC "/wd4275" "/wd4251")
+    # Disable diamond inheritance warnings
+    target_compile_options(${LIB_TARGET} PRIVATE "/wd4250")
+    ...
+  endif()
+endforeach()
+
 if(WIN32 AND ARROW_BUILD_STATIC)
   target_compile_definitions(parquet_static PUBLIC PARQUET_STATIC)
 endif()

We can use PRIVATE for warnings that are only for Parquet source.
We can use PUBLIC for warnings that are for Parquet source and Parquet users.

behrisch · 2025-07-29T07:40:35Z

As you can see, adding pop to the header file is not sufficient. Since ParquetException is used almost by all Parquet source files, we will end up with adding pop to every source file which seems meaningless if the warning can't be fixed.

I don't think so. We just need to add another pair of push/pop to exception.h. I can give this a try if you wish

wgtmac · 2025-07-29T10:36:11Z

Sure, feel free to try it out. Thanks!

adding pragma warning(pop) to platform.h

e902761

behrisch requested a review from wgtmac as a code owner July 15, 2025 15:55

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Jul 15, 2025

adding PARQUET_EXPORT as recommended

008203a

Disabling warnings for exception.h

d1cc446

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-47099: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47114

GH-47099: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47114

behrisch commented Jul 15, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Jul 15, 2025

Uh oh!

wgtmac commented Jul 16, 2025

Uh oh!

behrisch commented Jul 19, 2025

Uh oh!

wgtmac commented Jul 21, 2025

Uh oh!

behrisch commented Jul 24, 2025

Uh oh!

wgtmac commented Jul 24, 2025

Uh oh!

behrisch commented Jul 24, 2025

Uh oh!

wgtmac commented Jul 25, 2025

Uh oh!

kou commented Jul 25, 2025

Uh oh!

behrisch commented Jul 29, 2025

Uh oh!

wgtmac commented Jul 29, 2025

Uh oh!

Uh oh!

GH-47099: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47114

Are you sure you want to change the base?

GH-47099: [C++][Parquet] parquet/platform.h is missing pragma warning(pop) #47114

Conversation

behrisch commented Jul 15, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jul 15, 2025

Uh oh!

wgtmac commented Jul 16, 2025

Uh oh!

behrisch commented Jul 19, 2025

Uh oh!

wgtmac commented Jul 21, 2025

Uh oh!

behrisch commented Jul 24, 2025

Uh oh!

wgtmac commented Jul 24, 2025

Uh oh!

behrisch commented Jul 24, 2025

Uh oh!

wgtmac commented Jul 25, 2025

Uh oh!

kou commented Jul 25, 2025

Uh oh!

behrisch commented Jul 29, 2025

Uh oh!

wgtmac commented Jul 29, 2025

Uh oh!

Uh oh!

behrisch commented Jul 15, 2025 •

edited by github-actions bot

Loading