Skip to content

Conversation

@fanquake
Copy link
Member

We don't include strings.h anywhere.

This is also already checked for by autoconf, so us checking for it just means a 3rd existence check during ./configure.

We don't include strings.h anywhere.

This is also already checked for by autoconf, so us checking for it just
means a 3rd existence check during ./configure.
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

Code review ACK eb6026b

Indeed, strings.h is unused and is checked (excluded) in AC_INCLUDES_DEFAULT

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK eb6026b, tested on macOS 12.6:

% ./autogen.sh
% ./configure 2>&1 | grep -e 'strings.h'
checking for strings.h... yes
checking for strings.h... (cached) yes
checking for strings.h... yes
  • with this PR:
% ./autogen.sh
% ./configure 2>&1 | grep -e 'strings.h'       
checking for strings.h... yes
checking for strings.h... yes

@fanquake fanquake merged commit 4e15a28 into bitcoin:master Sep 21, 2022
@fanquake fanquake deleted the remove_strings_h branch September 21, 2022 15:30
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 23, 2022
eb6026b build: remove strings.h from header checks (fanquake)

Pull request description:

  We don't include `strings.h` anywhere.

  This is also already checked for by autoconf, so us checking for it just means a 3rd existence check during `./configure`.

ACKs for top commit:
  TheCharlatan:
    Code review ACK eb6026b
  hebasto:
    ACK eb6026b, tested on macOS 12.6:

Tree-SHA512: 4036c21b2f659140e9f471b4d24336fe925c6c010e2ced36e1f606d9c76dea236d086d15a884eb8f95381b39322abeecab973b10532527005fdadd095411e358
@bitcoin bitcoin locked and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants