Skip to content

refactor: Drop noop gcc version checks #20491

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

Jump to bottom
Merged
merged 1 commit into from
Nov 30, 2020
Merged

refactor: Drop noop gcc version checks #20491

merged 1 commit into from
Nov 30, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 25, 2020

Since #20413 the minimum required GCC version is 7.

@fanquake
Copy link
Member

fanquake commented Nov 25, 2020 •
edited
Loading

I think these are included in #19183. Also, unless we added that check, I'm not sure if we want to modify nanobench.

Edit: Saying that. #19183 is being split up/possibly blocked, so if the nanobench change is removed, I don't mind if this is merged earlier.

@hebasto @practicalswift
Drop noop gcc version checks
830ddf4 
Since bitcoin#20413 the minimum required GCC version is 7.

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
@hebasto
Copy link
Member Author

hebasto commented Nov 25, 2020

I think these are included in #19183. Also, unless we added that check, I'm not sure if we want to modify nanobench.

Edit: Saying that. #19183 is being split up/possibly blocked, so if the nanobench change is removed, I don't mind if this is merged earlier.

Removed nanobench change, and added credits to @practicalswift.

@maflcko maflcko changed the title Drop noop gcc version checks refactor: Drop noop gcc version checks Nov 25, 2020
maflcko
maflcko reviewed Nov 25, 2020
View reviewed changes
src/test/fuzz/multiplication_overflow.cpp
@@ -14,7 +14,7 @@
#if __has_builtin(__builtin_mul_overflow)
#define HAVE_BUILTIN_MUL_OVERFLOW
#endif
#elif defined(__GNUC__) && (__GNUC__ >= 5)
#elif defined(__GNUC__)
Copy link
Member

Choose a reason for hiding this comment

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

isn't this covered by the previous check already?

https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Integer-Overflow-Builtins.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted back due to @fanquake's comment.

@laanwj
Copy link
Member

laanwj commented Nov 25, 2020

Code review ACK a526ab4

@fanquake
Copy link
Member

I'm not sure this is correct, or at-least, now that this is only using __has_builtin() for the builtin checks, they wont be defined for GCC < 10, as that was the first version to get __has_builtin().

New built-in functions:
The __has_builtin built-in preprocessor operator can be used to query support for built-in functions provided by GCC and other compilers that support it.

Is that the desired behavior? I assume that's why it also has a check for __GNUC__.

i.e:

#include <iostream>

int main() {
#if defined(__has_builtin)
        std::cout << "has __has_builtin()\n";
#endif
        return 0;
}
g++ --version
g++ (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

g++ test.cpp && ./a.out
# not avaliable

g++-10 test.cpp && ./a.out
has __has_builtin()

@hebasto
Copy link
Member Author

hebasto commented Nov 28, 2020

I'm not sure this is correct, or at-least, now that this is only using __has_builtin() for the builtin checks, they wont be defined for GCC < 10, as that was the first version to get __has_builtin().

Updated (reverted back).

fanquake
fanquake approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 830ddf4

@fanquake fanquake merged commit 817aeca into bitcoin:master Nov 30, 2020
@hebasto hebasto deleted the 201125-gcc branch November 30, 2020 08:47
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 30, 2020
@fanquake
Merge bitcoin#20491: refactor: Drop noop gcc version checks
888a0d5 
830ddf4 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since bitcoin#20413 the minimum required GCC version is 7.

ACKs for top commit:
  fanquake:
    ACK 830ddf4

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
@barton2526 barton2526 mentioned this pull request Aug 13, 2021
Merged
christiancfifi pushed a commit to christiancfifi/dash that referenced this pull request Oct 3, 2021
@fanquake @christiancfifi
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
116dcb6 
830ddf4 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since bitcoin#20413 the minimum required GCC version is 7.

ACKs for top commit:
  fanquake:
    ACK 830ddf4

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
christiancfifi pushed a commit to christiancfifi/dash that referenced this pull request Oct 4, 2021
@fanquake @christiancfifi
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
3ab1378 
830ddf4 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since bitcoin#20413 the minimum required GCC version is 7.

ACKs for top commit:
  fanquake:
    ACK 830ddf4

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
@christiancfifi christiancfifi mentioned this pull request Oct 4, 2021
Merged
christiancfifi pushed a commit to christiancfifi/dash that referenced this pull request Oct 11, 2021
@fanquake @christiancfifi
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
e447c4e 
830ddf4 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since bitcoin#20413 the minimum required GCC version is 7.

ACKs for top commit:
  fanquake:
    ACK 830ddf4

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@fanquake @pravblockc
Partial Merge bitcoin#20491: refactor: Drop noop gcc version checks
8d0121b 
830ddf4 Drop noop gcc version checks (Hennadii Stepanov)

Pull request description:

  Since bitcoin#20413 the minimum required GCC version is 7.

ACKs for top commit:
  fanquake:
    ACK 830ddf4

Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

@practicalswift practicalswift Awaiting requested review from practicalswift

+1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hebasto @fanquake @laanwj @maflcko