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

made it possible to create a TokenList from a buffer #347

Merged
merged 3 commits into from
May 24, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

Still need to integrate/test it with #261 to make sure it does not introduce any issues.

It is already actively used by Macro though and no tests are failing, so it should be fine.

@firewave
Copy link
Collaborator Author

Some tests are still failing - will investigate.

@firewave
Copy link
Collaborator Author

The tests failing are the ones with binary characters in the string. We need to add an additional const char*, int input for those.

@firewave firewave changed the title added StdStringStream which reads from a std::string added constructors to create a TokenList from a abuffer Mar 29, 2024
@firewave firewave changed the title added constructors to create a TokenList from a abuffer added constructors to create a TokenList from a buffer Mar 29, 2024
@firewave
Copy link
Collaborator Author

I changed it from a std::string input to a const unsigned char*, std::size_t and const char*, std::size_t (for convienience).

The tests pass but it produces valgrind failures in #261.

@firewave
Copy link
Collaborator Author

The tests pass but it produces valgrind failures in #261.

The issue has been fixed - see https://github.com/danmar/simplecpp/actions/runs/8484373804/job/23247192053?pr=261.

@firewave firewave marked this pull request as ready for review March 29, 2024 18:43
@firewave firewave marked this pull request as draft March 29, 2024 18:43
@firewave firewave changed the title added constructors to create a TokenList from a buffer made it possible to create a TokenList from a buffer Mar 29, 2024
@firewave firewave marked this pull request as ready for review March 29, 2024 22:06
@firewave
Copy link
Collaborator Author

I will provide a downstream PR which shows this is working within Cppcheck.

We should also do the (planned) release before merging this.

@danmar
Copy link
Owner

danmar commented May 3, 2024

@firewave would you like that I release simplecpp now or do you have some more fixes..?

@firewave firewave marked this pull request as draft May 3, 2024 13:12
@firewave
Copy link
Collaborator Author

firewave commented May 3, 2024

@firewave would you like that I release simplecpp now or do you have some more fixes..?

Do the release so we are in proper sync again.

Afterwards this PR, #339 and #312 require your feedback - and some minor work from my side.

}

virtual int get() OVERRIDE {
if (pos >= size)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These checks are still a hot spot. No idea if there is a way to implement this in a more performant way.

@firewave
Copy link
Collaborator Author

firewave commented May 3, 2024

Here is the downstream PR using char buffers instead of streams: danmar/cppcheck#6379.

@firewave
Copy link
Collaborator Author

Ready for review since the downstream PR passes all tests.

@firewave firewave marked this pull request as ready for review May 16, 2024 13:51
simplecpp.cpp Show resolved Hide resolved
simplecpp.cpp Show resolved Hide resolved
simplecpp.cpp Show resolved Hide resolved
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

lgtm

@danmar
Copy link
Owner

danmar commented May 23, 2024

@firewave you should have gotten an invite for this project. I hope you can merge..

@firewave
Copy link
Collaborator Author

you should have gotten an invite for this project. I hope you can merge..

@danmar Thanks.

@firewave firewave merged commit d904016 into danmar:master May 24, 2024
14 checks passed
@firewave firewave deleted the stdstringstream branch May 24, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants