Skip to content

Redmine 6334 v2#15341

Closed
0x-0ddc0de wants to merge 2 commits into
OISF:mainfrom
0x-0ddc0de:redmine-6334-v2
Closed

Redmine 6334 v2#15341
0x-0ddc0de wants to merge 2 commits into
OISF:mainfrom
0x-0ddc0de:redmine-6334-v2

Conversation

@0x-0ddc0de
Copy link
Copy Markdown

Update to address comments on Previous PR. Ran clang-format.

Issue: 6334

@0x-0ddc0de 0x-0ddc0de requested a review from victorjulien as a code owner May 8, 2026 20:28
Comment thread src/util-spm.c
if (found == 0) {
printf("Error1 searching for %s in text %s\n", needle[i], text[i][j]);
return 0;
FAIL;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the expectation here is to replace the whole if (found == 0) { ... block with a single FAIL_IF(found == 0);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure I can do that. I had originally considered doing just as you described; however, it seemed that removing the printf makes it hard to trace down which specific subcase was failing, which is why I left the printf.

Copy link
Copy Markdown
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Would like to see the no-branch idea followed through more thoroughly. Also make sure to not create multiple commits that are having the exact same text. In this case I would expect a single commit.

@github-actions
Copy link
Copy Markdown

NOTE: This PR may contain new authors.

@0x-0ddc0de
Copy link
Copy Markdown
Author

Would like to see the no-branch idea followed through more thoroughly. Also make sure to not create multiple commits that are having the exact same text. In this case I would expect a single commit.

Done. My v3 PR squashes into a single commit. I had misunderstood the docs on this, as it described creating a new PR for each revision.

@jufajardini
Copy link
Copy Markdown
Contributor

Replaced by: #15368

Didn't investigate the CI failures. Maybe missing a rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants