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

tr: correctly handle multibyte octal sequences #6779

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

andrewliebenow
Copy link
Contributor

No description provided.

@andrewliebenow
Copy link
Contributor Author

Partly resolves the last issue listed in #6777.

I'm not sure how to perform logging inside the nom parsing code without the messages being printed twice.

Copy link

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

@andrewliebenow
Copy link
Contributor Author

First and last issues in #6777 are now addressed by this

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

Copy link

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/tail/assert is no longer failing!

@RenjiSann
Copy link
Contributor

RenjiSann commented Oct 24, 2024

Hi, I've played a bit with octal parsing as well, and I come to the same conclusion as you, which is that the logging of a warning is impossible to do because it is executed several times.

The solutions I see are the following:

  • Use some sort of global value to memoize the fact that this warning was already displayed (which is a terrible idea)
  • Introduce a lexer before the parser to handle escape sequences (which is nice, but is also a much bigger change)

What do you think ?

(my changes for reference)

@andrewliebenow
Copy link
Contributor Author

@RenjiSann The least-bad idea I could think of was changing the return type of all of these functions to allow returning a warning to print, but that would be kind of ugly considering that it's only this one path that would actually return one.

I should probably remove my commented-out code, so we can at least get the main fix merged in (albeit without the warning).

@RenjiSann
Copy link
Contributor

RenjiSann commented Oct 24, 2024

@RenjiSann The least-bad idea I could think of was changing the return type of all of these functions to allow returning a warning to print, but that would be kind of ugly considering that it's only this one path that would actually return one.

I am not convinced this would efficiently solve the problem, it might be over-complicated to check for different warning messages that should both be displayed

I should probably remove my commented-out code, so we can at least get the main fix merged in (albeit without the warning).

Let's do this, and maybe open an issue for handling the warning correctly later

@andrewliebenow andrewliebenow force-pushed the tr-multibyte-octal-sequences branch 2 times, most recently from a7b32d3 to 186d749 Compare October 30, 2024 04:32
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@sylvestre sylvestre merged commit 566bca3 into uutils:main Oct 30, 2024
67 of 68 checks passed
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.

3 participants