Skip to content

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Oct 7, 2024

No description provided.

@@ -48,18 +47,30 @@ class TestPreprocessor : public TestFixture {
TestPreprocessor() : TestFixture("TestPreprocessor") {}

private:
static std::string expandMacros(const char code[], ErrorLogger &errorLogger) {
std::istringstream istr(code);
template<size_t size>
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see the advantage at all here neither. What exactly is the advantage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do not need the stream and get the size.

Copy link
Owner

Choose a reason for hiding this comment

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

I am not very convinced about this. It feels like using a template and complicating the code just for fun.
imho it's a bit weird syntax const char (&code)[size] we could have used a strlen or a std::string. this is test code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not for fun. Some code in the core is just provided to accommodate the tests and that should be get rid of (and will be done in other changes).

And we run the test code mostly in debug i.e. no optimizations so it is slower than production code and thus having slower code for convenience should be avoided. Makes me wonder if unoptimized matchcompiled code might behave better.

@firewave firewave marked this pull request as draft August 20, 2025 12:54
@firewave firewave changed the title TestPreprocessor: some cleanups TestPreprocessor: removed usage of std::istringstream Aug 20, 2025
@firewave firewave marked this pull request as ready for review August 20, 2025 13:05
Copy link

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.

I don't like it really. I feel you uglify/complicate the testcode.

@@ -48,18 +47,30 @@ class TestPreprocessor : public TestFixture {
TestPreprocessor() : TestFixture("TestPreprocessor") {}

private:
static std::string expandMacros(const char code[], ErrorLogger &errorLogger) {
std::istringstream istr(code);
template<size_t size>
Copy link
Owner

Choose a reason for hiding this comment

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

I am not very convinced about this. It feels like using a template and complicating the code just for fun.
imho it's a bit weird syntax const char (&code)[size] we could have used a strlen or a std::string. this is test code.

@firewave
Copy link
Collaborator Author

I don't like it really. I feel you uglify/complicate the testcode.

It only "uglifies" the functions you call and in most cases it does not have much on the tests themselves. As seen in #6379 it helps with getting rid of unnecessary (slow) wrappers and overloads in the production code. I did not adjust tests where it really makes things worse.

There's also a simplecpp PR which makes things better.

@firewave firewave merged commit 90ee464 into danmar:main Aug 22, 2025
63 checks passed
@firewave firewave deleted the preproc-xxx-3 branch August 22, 2025 13:37
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