Skip to content

Commit

Permalink
GH-35596: [C++][CI] Improve compilation caching with PCG (#35597)
Browse files Browse the repository at this point in the history
### Rationale for this change

The PCG headers use the `__DATE__` and `__TIME__` macros, which makes builds non-deterministic:
imneme/pcg-cpp#59

Deterministic builds are useful for a number of reasons. One is security audits of binary artifacts, another (that directly affects us) is making compilation caching as efficient as possible. Preprocessor-based compilation caching breaks when time-dependent macros are used.

### What changes are included in this PR?

Remove the `struct static_arbitrary_seed` construct from PCG.
Also, enable a gcc/clang warning that detects the use of non-deterministic preprocessor macros.

### Are these changes tested?

Yes, by the additional warning (which is turned into an error in "checkin" warnings mode).

### Are there any user-facing changes?

No.

* Closes: #35596

Authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
  • Loading branch information
pitrou authored May 16, 2023
1 parent 8be70c1 commit f6e4479
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 21 deletions.
2 changes: 2 additions & 0 deletions cpp/cmake_modules/SetupCxxFlags.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,13 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unused-parameter")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-constant-logical-operand")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-return-stack-address")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-conversion")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-sign-conversion")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel")
if(WIN32)
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/vendored/pcg/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ Sources are taken from git changeset ffd522e7188bef30a00c74dc7eb9de5faff90092

Changes:
- enclosed in `arrow_vendored` namespace
- remove `struct arbitrary_seed` definition because of https://github.com/apache/arrow/issues/35596

21 changes: 0 additions & 21 deletions cpp/src/arrow/vendored/pcg/pcg_extras.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -612,27 +612,6 @@ class seed_seq_from {
}
};

/*
* Sometimes you might want a distinct seed based on when the program
* was compiled. That way, a particular instance of the program will
* behave the same way, but when recompiled it'll produce a different
* value.
*/

template <typename IntType>
struct static_arbitrary_seed {
private:
static constexpr IntType fnv(IntType hash, const char* pos) {
return *pos == '\0'
? hash
: fnv((hash * IntType(16777619U)) ^ *pos, (pos+1));
}

public:
static constexpr IntType value = fnv(IntType(2166136261U ^ sizeof(IntType)),
__DATE__ __TIME__ __FILE__);
};

// Sometimes, when debugging or testing, it's handy to be able print the name
// of a (in human-readable form). This code allows the idiom:
//
Expand Down

0 comments on commit f6e4479

Please sign in to comment.