Skip to content

Enable clojure.string-test.reverse#882

Merged
jeaye merged 11 commits into
jank-lang:mainfrom
djblue:reverse
Jul 14, 2026
Merged

Enable clojure.string-test.reverse#882
jeaye merged 11 commits into
jank-lang:mainfrom
djblue:reverse

Conversation

@djblue

@djblue djblue commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Before:

% jank repl
user=> (require '[clojure.string :as str])
nil
user=> (str/reverse "hello")
"olleh"
user=> (str/reverse "🫲  hello 🫱")
"���� olleh  ����"

After:

% jank repl              
user=> (require '[clojure.string :as str])
nil
user=> (str/reverse "hello")
"olleh"
user=> (str/reverse "🫲  hello 🫱")
"🫱 olleh 🫲"

Comment thread compiler+runtime/src/cpp/clojure/string_native.cpp Outdated
@djblue
djblue requested a review from jeaye July 5, 2026 06:40
Comment thread compiler+runtime/src/cpp/clojure/string_native.cpp

@jeaye jeaye left a comment

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.

Thank you, Chris!

Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
Comment thread compiler+runtime/include/cpp/jtl/utf8.hpp Outdated
@djblue
djblue requested a review from jeaye July 12, 2026 09:38

@jeaye jeaye left a comment

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.

Thanks, Chris!

Comment on lines +23 to +24
utf8_iterator begin() const;
utf8_iterator end() const;

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.

Iterators do not have begin/end. Ranges do. These two things should not be conflated.

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.

Below, when you use this with a reverse view, what we actually want is utf8_range{ s } | .... So the fix here is to add a new struct utf8_range which holds a string and provides begin/end (and optionally rbegin/rend).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for all the code churn. I'm still learning about iterators + range based for loops. I think I finally landed in a good place. Initially we talked about having two separate iterators forwards/reverse, but it felt better if we could have one iterator that could go forwards and backwards.

jeaye
jeaye previously approved these changes Jul 13, 2026

@jeaye jeaye left a comment

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.

Nice!

@jeaye

jeaye commented Jul 13, 2026

Copy link
Copy Markdown
Member

Merged jank-lang/clojure-test-suite#936. Want to pull that in here?

@djblue

djblue commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@jeaye when I bump to latest clojure-test-suite, existing enabled tests break. Should we address bumping clojure-test-suite in another PR, or should I disable the failing tests?

@jeaye

jeaye commented Jul 13, 2026

Copy link
Copy Markdown
Member

@jeaye when I bump to latest clojure-test-suite, existing enabled tests break. Should we address bumping clojure-test-suite in another PR, or should I disable the failing tests?

Let's disable the failing tests.

@jeaye jeaye left a comment

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.

Nice!

@jeaye
jeaye merged commit 05af54b into jank-lang:main Jul 14, 2026
17 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.

2 participants