Skip to content
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

Log all but string and char as hex #100

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Log all but string and char as hex #100

wants to merge 18 commits into from

Conversation

Christian-B
Copy link
Member

@Christian-B Christian-B commented Sep 10, 2019

Do not merge until SpiNNakerManchester/SpiNNakerGraphFrontEnd#125 is Green!

Note: @oliverrhodes and @lplana would be against the complete removal of the ability to print values directly to iobuf during debugging. He is mainly interested in ints and accums. He does not mind if the abilty to format these values is removed.

I would not recommend merging this until.

  1. We have a matching PR on spin tools which shows a clear saving in ITCM
  2. We have the single dict file implemented

@Christian-B Christian-B added enhancement experimental Do not commit this PR yet! labels Sep 10, 2019
@Christian-B Christian-B added this to the 5.1.0 milestone Sep 10, 2019
@lplana
Copy link
Member

lplana commented Sep 10, 2019

As I mentioned to @Christian-B and @dkfellows, I don't think that there is a need to get rid of 'io_printf()' and the ability to print values directly to 'IO_BUF'. 'io_printf()' is quite stable and requires little or no maintenance.

If we got it right (big if), the linker will not link in any function that is not called, thus 'io_printf()' will not be a part of the executable file if it is not used in the application. The user can choose to use it or not, depending on their needs and the size of their executable.

The case may be that the 'logging' infrastructure uses 'io_printf()'. In this case, 'logging' should stop using 'io_printf()' and use a 'hex-based' trimmed-down version, as implemented in this pull request. In this case, 'io_printf()' would not be linked in and the executable would actually benefit from the size reduction, but users like me would still have the option of using 'io_printf()' and read 'IO_BUF' directly, without the need to consult the dictionary.

@coveralls
Copy link

coveralls commented Sep 16, 2019

Pull Request Test Coverage Report for Build 924

  • 34 of 46 (73.91%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 92.229%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spinn_utilities/make_tools/replacer.py 24 36 66.67%
Files with Coverage Reduction New Missed Lines %
spinn_utilities/make_tools/replacer.py 1 80.82%
Totals Coverage Status
Change from base Build 922: 0.3%
Covered Lines: 4890
Relevant Lines: 5302

💛 - Coveralls

@dkfellows dkfellows modified the milestones: 5.1.0, 6.0.0 Nov 22, 2019
@Christian-B Christian-B modified the milestones: 6.0.0, Bluesky Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement experimental Do not commit this PR yet!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants