Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Apr 7, 2021

Similar to #10843. We could build with -Wmissing-noreturn, however that would also mean modifying something like --suppress-external-warnings to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.

@practicalswift
Copy link
Contributor

Concept ACK

@fanquake fanquake force-pushed the build_with_noreturn branch from 122b6e5 to d154478 Compare April 8, 2021 04:06
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK d1544784e972918a3a7d803bae55e5a4743fbf27

nit: s/attributes/attribute/ in the commit message, it is just one addition :)

I think extending --suppress-external-warnings to cover leveldb is ok - that code is not in our control, low quality there should not drag Bitcoin Core quality down. And --suppress-external-warnings is optional.

@fanquake fanquake force-pushed the build_with_noreturn branch from d154478 to 003929c Compare April 13, 2021 00:59
@fanquake
Copy link
Member Author

nit: s/attributes/attribute/ in the commit message, it is just one addition :)

Fixed.

I think extending --suppress-external-warnings to cover leveldb is ok

I'll look at doing this. We are probably also overdue for a leveldb subtree update.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

ACK 003929c

@fanquake fanquake merged commit 88331aa into bitcoin:master Apr 13, 2021
@fanquake fanquake deleted the build_with_noreturn branch April 13, 2021 13:18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2021
…cable

003929c refactor: add [[noreturn]] attribute where applicable (fanquake)

Pull request description:

  Similar to bitcoin#10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.

ACKs for top commit:
  vasild:
    ACK 003929c

Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…cable

003929c refactor: add [[noreturn]] attribute where applicable (fanquake)

Pull request description:

  Similar to bitcoin#10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.

ACKs for top commit:
  vasild:
    ACK 003929c

Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 16, 2021
…cable

003929c refactor: add [[noreturn]] attribute where applicable (fanquake)

Pull request description:

  Similar to bitcoin#10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.

ACKs for top commit:
  vasild:
    ACK 003929c

Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
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