Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kou merged 8 commits into from
Oct 12, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
99ac3bb
adding pragma warning(pop) to platform.h
behrisch Jul 15, 2025
2c4a4a5
adding PARQUET_EXPORT as recommended
behrisch Jul 16, 2025
36a9bd9
Disabling warnings for exception.h
behrisch Jul 29, 2025
eb7be0b
re-adding export which was missing in previous commit to exception.h
behrisch Jul 30, 2025
c3d2396
another export for hasher.h
behrisch Jul 30, 2025
323275c
disabling diamond pattern warnings
behrisch Jul 31, 2025
6e05b9b
removing redundant check for windows platform #47114
behrisch Aug 25, 2025
0f4a6ab
using short form of ifdef
behrisch Sep 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cpp/src/parquet/column_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@
#include "parquet/thrift_internal.h" // IWYU pragma: keep
#include "parquet/windows_fixup.h" // for OPTIONAL

#ifdef _MSC_VER
Copy link
Member

@wgtmac wgtmac Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move it into parquet/windows_fixup.h?

Copy link
Contributor Author

@behrisch behrisch Aug 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether this is a good idea. parquet/windows_fixup.h is again part of the public facing API (via parquet/schema.h) so we would need to do push / pop again to avoid disabling the warning for everyone.

Copy link
Member

@wgtmac wgtmac Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of defining some common macros for these but it seems that these patterns have already been scattered around. WDYT? @kou

Copy link
Member

@kou kou Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about creating parquet/windows_fixup_internal.h?
Our build system doesn't install *_internal.h.

Copy link
Contributor Author

@behrisch behrisch Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether it is wort the effort. We are talking about two times three lines of code. Does this need a separate file?

// disable warning about inheritance via dominance in the diamond pattern
# pragma warning(disable : 4250)
#endif

using arrow::MemoryPool;
using arrow::internal::AddWithOverflow;
using arrow::internal::checked_cast;
Expand Down
5 changes: 5 additions & 0 deletions cpp/src/parquet/decoder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@
#include "parquet/schema.h"
#include "parquet/types.h"

#ifdef _MSC_VER
// disable warning about inheritance via dominance in the diamond pattern
# pragma warning(disable : 4250)
#endif

namespace bit_util = arrow::bit_util;

using arrow::Status;
Expand Down
5 changes: 5 additions & 0 deletions cpp/src/parquet/encoder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@
#include "parquet/schema.h"
#include "parquet/types.h"

#ifdef _MSC_VER
// disable warning about inheritance via dominance in the diamond pattern
# pragma warning(disable : 4250)
#endif

namespace bit_util = arrow::bit_util;

using arrow::Status;
Expand Down
19 changes: 18 additions & 1 deletion cpp/src/parquet/exception.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@
#include "arrow/util/string_util.h"
#include "parquet/platform.h"

#ifdef _MSC_VER
# pragma warning(push)
// 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
# pragma warning(disable : 4275 4251)
// Disable diamond inheritance warnings
# pragma warning(disable : 4250)
// Disable macro redefinition warnings
# pragma warning(disable : 4005)
// Disable extern before exported template warnings
# pragma warning(disable : 4910)
#endif

// PARQUET-1085
#if !defined(ARROW_UNUSED)
# define ARROW_UNUSED(x) UNUSED(x)
Expand Down Expand Up @@ -84,7 +97,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";
Expand Down Expand Up @@ -156,3 +169,7 @@ void ThrowNotOk(StatusReturnBlock&& b) {
}

} // namespace parquet

#ifdef _MSC_VER
# pragma warning(pop)
#endif
2 changes: 1 addition & 1 deletion cpp/src/parquet/hasher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

namespace parquet {
// Abstract class for hash
class Hasher {
class PARQUET_EXPORT Hasher {
public:
/// Compute hash for 32 bits value by using its plain encoding result.
///
Expand Down
6 changes: 5 additions & 1 deletion cpp/src/parquet/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

#if defined(_WIN32) || defined(__CYGWIN__)

# if defined(_MSC_VER)
# ifdef _MSC_VER
# pragma warning(push)
// 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
Expand Down Expand Up @@ -110,3 +110,7 @@ std::shared_ptr<ResizableBuffer> AllocateBuffer(
::arrow::MemoryPool* pool = ::arrow::default_memory_pool(), int64_t size = 0);

} // namespace parquet

#ifdef _MSC_VER
# pragma warning(pop)
#endif
5 changes: 5 additions & 0 deletions cpp/src/parquet/types.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@

#include "generated/parquet_types.h"

#ifdef _MSC_VER
// disable warning about inheritance via dominance in the diamond pattern
# pragma warning(disable : 4250)
#endif

using arrow::internal::checked_cast;
using arrow::util::Codec;

Expand Down
Loading