util: Move error message formatting of NonFatalCheckError to cpp #25112

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2205-err-impl-🗡 changing 3 files +22 −16

maflcko commented at 7:19 AM on May 12, 2022: member

This allows to strip down the header file.
maflcko added the label Refactoring on May 12, 2022
maflcko force-pushed on May 12, 2022
maflcko force-pushed on May 12, 2022
DrahtBot cross-referenced this on May 24, 2022 from issue refactor: Split util/system into exception, shell, and fs-specific files by Empact
DrahtBot commented at 1:39 PM on May 24, 2022: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Count Reviewers

ACK 2 aureleoules, hebasto

Concept ACK 1 laanwj



Conflicts

No conflicts as of last run.
DrahtBot cross-referenced this on May 28, 2022 from issue Handle invalid hex encoding in ParseHex by maflcko
DrahtBot cross-referenced this on May 28, 2022 from issue refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) by maflcko
in src/util/check.h:8 in fa13e2f365 outdated
```
   4 | @@ -5,30 +5,23 @@
   5 |  #ifndef BITCOIN_UTIL_CHECK_H
   6 |  #define BITCOIN_UTIL_CHECK_H
   7 |  
   8 | -#if defined(HAVE_CONFIG_H)
```
laanwj commented at 12:33 PM on May 30, 2022:

Any idea what this bitcoin-config.h was needed for before?

maflcko commented at 12:39 PM on May 30, 2022:

This was added without explanation in commit 691c817b340 by @ryanofsky

ryanofsky commented at 8:49 PM on May 30, 2022:

This was added without explanation in commit 691c817 by @ryanofsky

I added the include there to define the PACKAGE_BUGREPORT macro referenced below (it was probably needed previously, but not obvious because of an earlier include)

maflcko commented at 7:12 AM on May 31, 2022:

Thx.

In this pull, I moved this to the cpp file, as the PACKAGE_BUGREPORT is only used there. iwyu is also happy on the changes here: https://cirrus-ci.com/task/4985805499269120?logs=ci#L7190
laanwj commented at 12:33 PM on May 30, 2022: member

Concept ACK
DrahtBot cross-referenced this on May 31, 2022 from issue refactor: Add LIFETIMEBOUND / -Wdangling-gsl to Assert() by maflcko
maflcko force-pushed on May 31, 2022
DrahtBot cross-referenced this on Jun 9, 2022 from issue refactor: add most of src/util to iwyu by fanquake
DrahtBot cross-referenced this on Jul 1, 2022 from issue [kernel 3c/n] Decouple validation cache initialization from `ArgsManager` by dongcarl
DrahtBot cross-referenced this on Jul 6, 2022 from issue fuzz: add low-level target for txorphanage by chinggg
DrahtBot added the label Needs rebase on Jul 7, 2022
maflcko force-pushed on Jul 7, 2022
DrahtBot removed the label Needs rebase on Jul 7, 2022
DrahtBot added the label Needs rebase on Jul 12, 2022
maflcko force-pushed on Jul 12, 2022
DrahtBot removed the label Needs rebase on Jul 12, 2022
maflcko force-pushed on Sep 12, 2022
DrahtBot cross-referenced this on Oct 12, 2022 from issue ci: Integrate `bitcoin-tidy` clang-tidy plugin by fanquake
DrahtBot added the label Needs rebase on Nov 3, 2022
maflcko force-pushed on Nov 14, 2022
maflcko force-pushed on Nov 14, 2022
DrahtBot removed the label Needs rebase on Nov 14, 2022
maflcko cross-referenced this on Nov 16, 2022 from issue Add `UNREACHABLE` macro by hebasto
hebasto commented at 9:33 AM on November 16, 2022: member

Concept ACK.
in src/util/check.cpp:10 in fadbc71e01 outdated
```
   6 | +#if defined(HAVE_CONFIG_H)
   7 | +#include <config/bitcoin-config.h>
   8 | +#endif
   9 |  
  10 |  #include <tinyformat.h>
  11 | +#include <util/check.h>
```
hebasto commented at 11:06 AM on November 16, 2022:

nit: Usually we put the header, which is directly related to the source file, at the top of the headers.

maflcko commented at 11:24 AM on November 16, 2022:

yes, done

Type	Count	Reviewers
ACK	2	aureleoules, hebasto
Concept ACK	1	laanwj

in src/util/check.h:24 in fadbc71e01 outdated

  31 |  T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion)
  32 |  {
  33 |      if (!(val)) {
  34 | -        throw NonFatalCheckError(
  35 | -            format_internal_error(assertion, file, line, func, PACKAGE_BUGREPORT));
  36 | +        throw NonFatalCheckError{assertion, file, line, func};

hebasto commented at 11:07 AM on November 16, 2022:

nit: The #24812 (review) could be addressed as well here.

maflcko commented at 11:23 AM on November 16, 2022:

thx, done

hebasto approved
hebasto commented at 11:07 AM on November 16, 2022: member

ACK fadbc71e010b32e23f158bd95cdf2f26c870e11f, I have reviewed the code and it looks OK, I agree it can be merged.
util: Move error message formatting of NonFatalCheckError to cpp
This allows to strip down the header file
2222ec71fd
maflcko force-pushed on Nov 16, 2022
hebasto approved
hebasto commented at 11:58 AM on November 16, 2022: member

re-ACK 2222ec71fdf573a15bb593fc0dd42d2d28ca5449, only rebased and suggested changes since my recent review.
aureleoules approved
aureleoules commented at 12:19 PM on November 16, 2022: member

ACK 2222ec71fdf573a15bb593fc0dd42d2d28ca5449
maflcko merged this on Nov 16, 2022
maflcko closed this on Nov 16, 2022
maflcko deleted the branch on Nov 16, 2022
sidhujag referenced this in commit 6031a314c2 on Nov 16, 2022
bitcoin locked this on Mar 6, 2024

Contributors

Labels

Linked (view graph)

#24812 util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro #26504 Add `UNREACHABLE` macro

util: Move error message formatting of NonFatalCheckError to cpp #25112

Reviews

Conflicts