-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Split apart text-only logging and binary logging. #29661
Split apart text-only logging and binary logging. #29661
Conversation
PR #29661: Size comparison from 762d3ab to 0c6082f Increases (13 builds for bl702, bl702l, cc32xx, cyw30739, psoc6)
Decreases (14 builds for bl702, bl702l, cc13x4_26x4, cc32xx, k32w, psoc6)
Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
0c6082f
to
17af39e
Compare
PR #29661: Size comparison from 3dfffe7 to 17af39e Increases (13 builds for bl702, bl702l, cc32xx, cyw30739, psoc6)
Decreases (14 builds for bl702, bl702l, cc13x4_26x4, cc32xx, k32w, psoc6)
Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
Text-only logging does not depend on Span, but binary logging (LogByteSpan) does. And Span.h depends on text-only logging. We were manually breaking the cycle caused by having both logging types in CHIPLogging.h by not including Span.h from CHIPLogging.h and forward-declaring Span instead. This fixes things so that we have a TextOnlyLogging.h (which is used by Span.h, via CodeUtils.h, and does not use Span at all), and a BinaryLogging.h which uses Span and includes Span.h.
17af39e
to
9f56e1b
Compare
PR #29661: Size comparison from 3dfffe7 to 9f56e1b Increases (16 builds for bl702, bl702l, cc32xx, cyw30739, linux, psoc6)
Decreases (18 builds for bl702, bl702l, cc13x4_26x4, cc32xx, k32w, linux, psoc6)
Full report (50 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg)
|
It might be less churn to move |
In our tree... I was aiming to not break any existing consumers, who are including CHIPLogging.h and then using ChipLogByteSpan. |
Text-only logging does not depend on Span, but binary logging (LogByteSpan) does. And Span.h depends on text-only logging. We were manually breaking the cycle caused by having both logging types in CHIPLogging.h by not including Span.h from CHIPLogging.h and forward-declaring Span instead. This fixes things so that we have a TextOnlyLogging.h (which is used by Span.h, via CodeUtils.h, and does not use Span at all), and a BinaryLogging.h which uses Span and includes Span.h.
Text-only logging does not depend on Span, but binary logging (LogByteSpan) does. And Span.h depends on text-only logging.
We were manually breaking the cycle caused by having both logging types in CHIPLogging.h by not including Span.h from CHIPLogging.h and forward-declaring Span instead. This fixes things so that we have a TextOnlyLogging.h (which is used by Span.h, via CodeUtils.h, and does not use Span at all), and a BinaryLogging.h which uses Span and includes Span.h.
Review note: git's default copy detection is failing here; a diff with
-B
or--break-rewrites
as part of the diff args is likely more readable.