GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — added `TokenList::Stream` class to wrap `std::istream` usage and implemented alternative C I/O version by firewave · Pull Request #244 · cppcheck-opensource/simplecpp

Conversation

@firewave

@firewave firewave commented Mar 7, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Reading the file via std::istream includes a considerable overhead caused by std::istream::sentry and others. Using C I/O instead reduces the "total Ir" usage by about 10%.

Testing with -q -Ilib/ -D__GNUC__ lib/valueflow.cpp:

Clang 13 271,013,439 -> 269,398,283 -> 244,353,123
GCC 11 -> -> ``

The intermediate value shows the improvement if we only change the actual input file. The last value if we also do it for all the includes which does not require you to use the new interface function. So we even if you do not change the application which uses this it would still result in a sizable improvement.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@firewave

firewave commented Mar 7, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Still needs the unit tests to be adjusted to test both implementations as well as making it properly selectable in the standalone binary.

This can possibly be introduced further by adding buffering to FileStream.

We could also add a MemStream version which works on a memory buffer allowing for the use case of just a passing a std::istringstream (i.e. string) and deprecating/removing the old API with std::istream.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave force-pushed the stream branch 2 times, most recently from dff9b46 to 4193e57 Compare March 10, 2022 09:19
@firewave firewave mentioned this pull request Mar 15, 2022
even more testrunner cleanups #254
Merged
@firewave
firewave force-pushed the stream branch 3 times, most recently from 791e5b1 to 1781870 Compare April 19, 2022 07:40
@firewave

firewave commented Apr 19, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Somehow using fgetc() seems to already interpret the characters causing 0x00 to be omitted when read even though the file is open in "binary" mode. Using fread() (which is supposed to be faster) fixes this but it is much slower.

Turns out it's just a bug in my code with unget() and multi-byte characters.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave force-pushed the stream branch 3 times, most recently from 5fb149d to 1e8ee57 Compare April 20, 2022 07:21
@firewave
firewave force-pushed the stream branch from 1e8ee57 to 8b5522e Compare October 6, 2022 00:14
@firewave firewave mentioned this pull request Oct 6, 2022
test.cpp: also run tests with char buffer #261
Merged
@firewave

firewave commented Oct 27, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

I backed out the test changes and will merge them into #261 so they don't get lost.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave force-pushed the stream branch from a1dd44f to 3a582d3 Compare October 27, 2022 11:36
@firewave

firewave commented Oct 27, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Testing with -q -Ilib/ -D__GNUC__ lib/valueflow.cpp:

current -> std::istream input -> FILE* input
Clang 14 263,400,261 -> 255,772,481 -> 241,915,419
GCC 12 266,098,949 -> 261,699,461 -> 247,682,262

Testing with -q -Ilib/ -D__GNUC__ lib/tokenize.cpp:

current -> std::istream input -> FILE* input
Clang 14 300,502,647 -> 294,632,802 -> 275,871,698
GCC 12 304,202,739 -> 301,878,425 -> 282,870,756

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave force-pushed the stream branch 2 times, most recently from bbeb171 to 23217fc Compare November 20, 2022 21:37
@firewave
firewave force-pushed the stream branch from 23217fc to 1e8f324 Compare December 7, 2022 10:35
@danmar

danmar commented Dec 18, 2022

Copy link
Copy Markdown
Collaborator

reducing the Ir count by using C I/O sounds good to me.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave force-pushed the stream branch from 1e8f324 to f7f4108 Compare December 29, 2022 15:34
firewave added 9 commits February 25, 2023 16:56
@firewave
added wrapper class "Stream" to TokenList
097a765
@firewave
moved some helper function into TokenList::Stream
c2ae449
@firewave
simplified UTF-16 checks
62acc58
@firewave
made simplecpp::TokenList::Stream an abstract class and moved impleme…
a2cc676
…ntation-specific parts into StdIStream
@firewave
added simplecpp::TokenList::Stream implementation "FileStream" which …
07ec136
…uses C I/O functions
@firewave
use FileStream for includes as well
81fe8cd
@firewave
constness
4f0da78
@firewave
made some TokenList::Stream members private
1b52b22
@firewave
pulled out repeated UTF-16 character generation code
95c747b
firewave added 11 commits February 25, 2023 16:56
@firewave
main.cpp: added command-option "-is" to specify usage of std::istream…
892183e
… interface in reading initial file
@firewave
test.cpp: got rid of another std::istringstream usage
26ccd24
@firewave
fixed FileStream::unget() with subsequent calls (i.e. UTF-16 encoding)
e57f312
@firewave
simplified newline handling in TokenList::Stream::readChar()
bdc7f3c
@firewave
fixed handling of incomplete UTF-8 BOM 0xefbbbf
76b94e3
@firewave
test.cpp: added tests for incomplete UTF sequences
cd084ae
@firewave
keep lastCh intact when using FileStream::peek()
797f899
@firewave
fixed uninitialized members in FileStream
c1da435
@firewave
.clang-tidy: disabled modernize-use-override warning
52b4b61
@firewave
fixed readability-inconsistent-declaration-parameter-name clean-tid…
7ff8c98
…y warnings
@firewave
fixed -Wshadow Clang compiler warning
c9605fe
@firewave
firewave force-pushed the stream branch from f7f4108 to c9605fe Compare February 25, 2023 15:56
@firewave
adjusted parameter order in added TokenList() constructor / added s…
557099c
…ome barebones documentation
@firewave

firewave commented Feb 25, 2023

Copy link
Copy Markdown
Collaborator Author

I put this into Cppcheck with the current and added TokenList construction and all tests passed on Windows and Linux. So this is finally ready for review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@firewave
firewave marked this pull request as ready for review February 25, 2023 16:23
@danmar
danmar merged commit 00d1f67 into cppcheck-opensource:master Mar 2, 2023
@firewave
firewave deleted the stream branch March 2, 2023 21:41
@firewave firewave mentioned this pull request Apr 13, 2023
parse files with simplecpp without providing a stream cppcheck-opensource/cppcheck#4955
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@firewave @danmar
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.