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

Implement 9 of 21 clojure.string vars #201

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Jan 29, 2025

Implements:

  • clojure.string/blank?
  • clojure.string/capitalize
  • clojure.string/ends-with?
  • clojure.string/escape
  • clojure.string/includes?
  • clojure.string/lower-case
  • clojure.string/reverse
  • clojure.string/starts-with?
  • clojure.string/upper-case

@djblue
Copy link
Contributor

djblue commented Jan 29, 2025

Very cool! If you start looking at regex, I started some of that work here. I'm still looking through various performance characteristic of different regex libraries.

@frenchy64 frenchy64 changed the title WIP: clojure.string Implement 9 of 21 clojure.string vars Feb 5, 2025
@frenchy64 frenchy64 marked this pull request as ready for review February 5, 2025 06:03
@frenchy64 frenchy64 requested review from djblue and jeaye February 5, 2025 06:13
@@ -0,0 +1,55 @@
(ns clojure-string.self-test
Copy link
Member

Choose a reason for hiding this comment

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

Please put these tests in the https://github.com/jank-lang/clojure-test repo instead. We should have one place for testing all of the official Clojure namespaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compiler+runtime/src/cpp/main.cpp Show resolved Hide resolved
#include <jank/runtime/core.hpp>
#include <jank/runtime/context.hpp>
#include <jank/runtime/behavior/callable.hpp>
#include <jank/runtime/visit.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

You're not using any visting fns, while this is an incredibly expensive include. Please audit all of the includes here to ensure they're needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still working on this.

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