Skip to content

Conversation

firewave
Copy link
Collaborator

This gets rid of all the explicit std::istringstream usage in the test cases so I can hopefully easily switch the implementation for the new API introduced with #244.

@firewave
Copy link
Collaborator Author

@danmar Can this be merged? I need this to finish up that other PR.

test.cpp Outdated
std::istringstream istr(sz == -1 ? std::string(code) : std::string(code,sz));
std::vector<std::string> files;
return simplecpp::TokenList(istr,files,std::string(),outputList).stringify();
return makeTokenList(code, files);
Copy link
Owner

Choose a reason for hiding this comment

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

this looks dangerous. the returned TokenList will reference the local files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. I didn't think about this.

We didn't detect this in Cppcheck - I have this enabled in my IDE and it did not report anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I filed https://trac.cppcheck.net/ticket/10975. It is impossible to detect the issue I introduced though since we have no idea what is done with the reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

test.cpp Outdated
outputList.clear();

readfile("A = u8R\"as\n(abc)as\"", -1, &outputList);
readfile("A = u8R\"as\n(abc)as\"", &outputList);
Copy link
Owner

Choose a reason for hiding this comment

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

indentation seems wrong on this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@danmar danmar merged commit e317815 into danmar:master Apr 13, 2022
@firewave firewave deleted the test-5 branch April 14, 2022 07:10
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