GH-47953: [C++] Remove Windows inclusion from `int_util_overflow.h` by pitrou · Pull Request #47950 · apache/arrow

pitrou · 2025-10-27T09:05:44Z

Rationale for this change

In int_util_overflow.h we're currently including (indirectly) some Windows header through windows_compatibility.h:

arrow/cpp/src/arrow/util/int_util_overflow.h

Lines 28 to 33 in f669b9f

    
           // "safe-math.h" includes <intsafe.h> from the Windows headers. 
        
           #include "arrow/util/windows_compatibility.h" 
        
           #include "arrow/vendored/portable-snippets/safe-math.h" 
        
           // clang-format off (avoid include reordering) 
        
           #include "arrow/util/windows_fixup.h" 
        
           // clang-format on

This was probably necessary when this was initially committed, but can produce problems when expanding the use of this header in our codebase, due to unwanted macro definitions, here is an example:

D:/a/arrow/arrow/cpp/src/parquet/arrow/schema.cc:78:45: error: expected unqualified-id
   78 |   return is_nullable ? Repetition::OPTIONAL : Repetition::REQUIRED;
      |                                             ^
D:/a/arrow/arrow/cpp/src/parquet/arrow/schema.cc:331:45: error: expected unqualified-id
  331 |       if (repetition != Repetition::OPTIONAL) {
      |                                             ^
2 errors generated.

What changes are included in this PR?

Simply remove the Windows inclusion. This seems to pass our current CI suite.

Are these changes tested?

Yes, by existing CI tests.

Are there any user-facing changes?

No.

GitHub Issue: [C++] int_util_overflow.h includes some Windows headers #47953

github-actions · 2025-10-27T09:06:37Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

pitrou · 2025-10-27T09:07:16Z

@github-actions crossbow submit windowscp313* test-build-vcpkg-win

github-actions · 2025-10-27T09:10:28Z

Revision: 1a66de4

Submitted crossbow builds: ursacomputing/crossbow @ actions-552c508701

Task	Status
test-build-vcpkg-win
wheel-windows-cp313-cp313-amd64
wheel-windows-cp313-cp313t-amd64

pitrou · 2025-10-27T09:35:35Z

@github-actions crossbow submit r-binary-packages verify-rc-source-windows

github-actions · 2025-10-27T09:37:55Z

Revision: 1a66de4

Submitted crossbow builds: ursacomputing/crossbow @ actions-c171f0b7b7

Task	Status
r-binary-packages
verify-rc-source-windows

pitrou · 2025-10-27T11:57:41Z

CI failures are unrelated.

WillAyd

lgtm

pitrou · 2025-10-27T14:03:07Z

Ok, this PR was apparently not enough:
https://github.com/apache/arrow/actions/runs/18843300813/job/53760740194?pr=47925

In file included from D:/a/_temp/msys64/mingw64/include/winnt.h:9,
                 from D:/a/_temp/msys64/mingw64/include/minwindef.h:163,
                 from D:/a/_temp/msys64/mingw64/include/windef.h:9,
                 from D:/a/_temp/msys64/mingw64/include/windows.h:69,
                 from D:/a/_temp/msys64/mingw64/include/rpc.h:16,
                 from D:/a/_temp/msys64/mingw64/include/wtypesbase.h:7,
                 from D:/a/_temp/msys64/mingw64/include/intsafe.h:14,
                 from D:/a/arrow/arrow/cpp/src/arrow/vendored/portable-snippets/safe-math.h:127,
                 from D:/a/arrow/arrow/cpp/src/arrow/util/int_util_overflow.h:29,
                 from D:/a/arrow/arrow/cpp/src/arrow/util/time.h:28,
                 from D:/a/arrow/arrow/cpp/src/arrow/util/formatting.h:37,
                 from D:/a/arrow/arrow/cpp/src/arrow/integration/json_internal.cc:47:
D:/a/arrow/arrow/cpp/src/arrow/integration/json_internal.cc: In function 'arrow::Result<const arrow::rapidjson::GenericObject<true, arrow::rapidjson::GenericValue<arrow::rapidjson::UTF8<> > > > arrow::internal::integration::json::{anonymous}::GetMemberObject(const RjObject&, const std::string&)':
D:/a/arrow/arrow/cpp/src/arrow/integration/json_internal.cc:904:20: error: 'const class arrow::rapidjson::GenericValue<arrow::rapidjson::UTF8<> >' has no member named 'GetObjectA'; did you mean 'GetObject'?
  904 |   return it->value.GetObject();
      |                    ^~~~~~~~~

conbench-apache-arrow · 2025-10-27T21:07:33Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 57c329f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 268 possible false positives for unstable benchmarks that are known to sometimes produce them.

…w.h` (apache#47950) ### Rationale for this change In `int_util_overflow.h` we're currently including (indirectly) some Windows header through `windows_compatibility.h`: https://github.com/apache/arrow/blob/f669b9fb6b71e74bb41496252709b494233de44d/cpp/src/arrow/util/int_util_overflow.h#L28-L33 This was probably necessary when this was initially committed, but can produce problems when expanding the use of this header in our codebase, due to unwanted macro definitions, here is an [example](https://github.com/apache/arrow/actions/runs/18754865884/job/53504089972?pr=47925): ``` D:/a/arrow/arrow/cpp/src/parquet/arrow/schema.cc:78:45: error: expected unqualified-id 78 | return is_nullable ? Repetition::OPTIONAL : Repetition::REQUIRED; | ^ D:/a/arrow/arrow/cpp/src/parquet/arrow/schema.cc:331:45: error: expected unqualified-id 331 | if (repetition != Repetition::OPTIONAL) { | ^ 2 errors generated. ``` ### What changes are included in this PR? Simply remove the Windows inclusion. This seems to pass our current CI suite. ### Are these changes tested? Yes, by existing CI tests. ### Are there any user-facing changes? No. * GitHub Issue: apache#47953 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

EXP: [C++] Try removing Windows fixups

1a66de4

github-actions bot added Component: C++ awaiting review Awaiting review labels Oct 27, 2025

pitrou mentioned this pull request Oct 27, 2025

[C++] Adopt alternative safe math approach #47926

Closed

pitrou changed the title ~~EXP: [C++] Try removing Windows fixups~~ GH-47953: [C++] Remove Windows inclusion from int_util_overflow.h Oct 27, 2025

pitrou marked this pull request as ready for review October 27, 2025 11:12

pitrou added the CI: Extra: C++ Run extra C++ CI label Oct 27, 2025

pitrou requested review from WillAyd and zanmato1984 October 27, 2025 11:13

WillAyd approved these changes Oct 27, 2025

View reviewed changes

pitrou merged commit 57c329f into apache:main Oct 27, 2025
63 of 69 checks passed

pitrou removed the awaiting review Awaiting review label Oct 27, 2025

pitrou mentioned this pull request Oct 27, 2025

[C++] int_util_overflow.h includes some Windows headers #47953

Closed

github-actions bot added the awaiting committer review Awaiting committer review label Oct 27, 2025

pitrou deleted the exp-int-safe branch October 27, 2025 13:47

pitrou mentioned this pull request Oct 27, 2025

GH-47924: [C++] Fix issues in CSV reader with invalid inputs #47925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-47953: [C++] Remove Windows inclusion from `int_util_overflow.h`#47950

GH-47953: [C++] Remove Windows inclusion from `int_util_overflow.h`#47950
pitrou merged 1 commit intoapache:mainfrom
pitrou:exp-int-safe

pitrou commented Oct 27, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

WillAyd left a comment

Uh oh!

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

conbench-apache-arrow bot commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// "safe-math.h" includes <intsafe.h> from the Windows headers.
	#include "arrow/util/windows_compatibility.h"
	#include "arrow/vendored/portable-snippets/safe-math.h"
	// clang-format off (avoid include reordering)
	#include "arrow/util/windows_fixup.h"
	// clang-format on

Conversation

pitrou commented Oct 27, 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 Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Oct 27, 2025

Uh oh!

conbench-apache-arrow bot commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pitrou commented Oct 27, 2025 •

edited by github-actions bot

Loading