fix: (str nil) => "" #264

bpiel · 2025-02-13T11:13:52Z

Closes #241

Here are a some notes on the approach I took and why it differed from what was expected for this ticket.

The original behavior:

clojure.core=> 
nil
clojure.core=> (str nil)
"nil"
clojure.core=> (str nil "hello" nil)
"nilhellonil"
clojure.core=> (println nil)
nil
nil
clojure.core=> (println "hello" nil nil)
hello nil nil
nil
clojure.core=> (prn "hello" nil nil)
"hello" nil nil
nil
clojure.core=> nil
nil

First-attempt changes:

// nil.cpp
...
  native_persistent_string const &nil::to_string() const
  {
    // static native_persistent_string const s{ "nil" };
    static native_persistent_string const s{ "" };
    return s;
  }

  native_persistent_string const &nil::to_code_string() const
  {
    // return to_string();
    static native_persistent_string const s{ "nil" };
    return s;
  }

  void nil::to_string(util::string_builder &buff) const
  {
    //fmt::format_to(std::back_inserter(buff), "nil");
    fmt::format_to(std::back_inserter(buff), "");
  }
...

Behavior after first attempt

clojure.core=> 
nil    ;; should be empty line
clojure.core=> (str nil)
""     ;; good
clojure.core=> (str nil "hello" nil)
"hello" ;; good
clojure.core=> (println "hello" nil nil)
hello  ;; should be: hello nil nil
nil
clojure.core=> (prn "hello" nil nil)
"hello" nil nil  ;; good
nil
clojure.core=> nil
nil    ;; good

After some time spent trying and failing to correct the above, I eventually went to the Clojure source:

Clojure str:
https://github.com/clojure/clojure/blob/clojure-1.11.1/src/clj/clojure/core.clj#L546

(defn str
  ...
  (^String [] "")
  (^String [^Object x]
   (if (nil? x) "" (. x (toString))))  ;; <<==== NOTE
  (^String [x & ys]
     ((fn [^StringBuilder sb more]
          (if more
            (recur (. sb  (append (str (first more)))) (next more))
            (str sb)))
      (new StringBuilder (str x)) ys)))

Taking inspiration from Clojure, I decided to implement this nil behavior in jank's str functions. In the results below, all but the first case show correct behavior. The failing case is not a regression.

New behavior

clojure.core=> 
nil  ;; should be empty line
clojure.core=> (str nil)
""
clojure.core=> (str nil "hello" nil)
"hello"
clojure.core=> (println nil)
nil
nil
clojure.core=> (println "hello" nil nil)
hello nil nil
nil
clojure.core=> (prn "hello" nil nil)
"hello" nil nil
nil
clojure.core=> nil
nil

Also, this had no effect on:
keyword differs from CLJ/S for non-strings
#246

compiler+runtime/src/cpp/jank/runtime/core/seq.cpp

frenchy64 · 2025-02-13T19:18:24Z

Thanks for tackling this Bill, could you fix these stylistic issues to help me see the approach without getting distracted?

frenchy64 · 2025-02-13T19:26:31Z

Also please add Closes #241 to the top of the PR description.

bpiel · 2025-02-13T19:56:51Z

@frenchy64 Ugh.. maybe this one test is stuck? Any way to restart it?

jeaye · 2025-02-13T20:00:07Z

@frenchy64 Ugh.. maybe this one test is stuck? Any way to restart it?

What's stuck?

bpiel · 2025-02-13T20:21:02Z

@jeaye Nothing now. It was running for 26 or so mins when I sent that. It probably resolved as soon as I did, of course.
thanks

jeaye · 2025-02-13T20:23:59Z

@jeaye Nothing now. It was running for 26 or so mins when I sent that. It probably resolved as soon as I did, of course. thanks

Static analysis is very slow. It's amazing what it finds, though. 🙂

frenchy64 · 2025-02-13T21:24:31Z

compiler+runtime/src/cpp/jank/runtime/core/seq.cpp

+        if(!is_nil(o))
+        {
+          runtime::to_string(o, buff);
+        }
        if(0 < sequence_length(typed_args))


Question for @jeaye: can we remove this conditional? persistent_list::fresh_seq() will return nullptr and short-circuit the for, then we just need to check the count once.

Yeah, that makes sense. The for loop with fresh_seq should handle this just fine.

jeaye

Nice work finding your way when I led you down the wrong path, Bill! 🙂

jeaye · 2025-02-13T22:41:50Z

compiler+runtime/src/jank/clojure/core.jank

+     (if (nil? o)
+       ""
+       (clojure.core-native/to-string o)))


We're already doing the nil check in the C++ code. The way I read it, this extra nil? check is not needed. Is that right?

The trick here is that this line is calling to-string, which does not have the nil check. I added the nil check to str. So, I think this is right.

compiler+runtime/src/cpp/jank/runtime/core/seq.cpp

frenchy64 · 2025-02-19T19:23:16Z

compiler+runtime/src/cpp/jank/runtime/core/seq.cpp

-          for(auto it(fresh->next_in_place()); it != nullptr; it = it->next_in_place())
+          runtime::to_string(o, buff);
+        }
+        for(auto it(typed_args->fresh_seq()); it != nullptr; it = it->next_in_place())


Maybe not urgent, but I think everything above the for could be pulled out of the visitor to reduce the generated code. Then the buff could be passed as an argument.

I have no idea if it makes a difference in practice, but I've seen other parts of the code pass in the argument explicitly rather than make a closure.

jank/compiler+runtime/src/cpp/jank/runtime/core.cpp

Lines 27 to 40 in 1a75d85

return visit_object(

[](auto const typed_l, auto const r) -> native_integer {

using L = typename decltype(typed_l)::value_type;

if constexpr(behavior::comparable<L>)

{

return typed_l->compare(*r);

}

else

{

throw std::runtime_error{ fmt::format("not comparable: {}", typed_l->to_string()) };

}

},

l,

r);

oh, just saw this follow-up comment

Thanks @bpiel! I'm more worried about generated code size here but maybe @jeaye has a preference between the closure and explicit arg.

There are two approaches here:

Use a closure (which Bill is currently doing with [&])

Pass the explicit arg

The difference impacts runtime perf. The reason is that the closure generates to a struct when the C++ compiler does its thing. This struct has members for the captured objects. Calling the closure requires creating an instance of that struct, passing in the captured data, and then calling the call operator on that instance. This becomes harder to inline.

On the other hand, if we just use a [](auto const typed_args, util::string_builder &buff) then we can pass buff in as an arg. The C++ compiler will generate a free function which can be more easily inlined. We could use auto &buff, but that means implicitly adding a second type parameter, which then impacts compile-time perf more.

Overall, the difference between the two is marginal and the cases where I've used the second have been due to the profiler saying it'd help. Given that str is a very common function, I think it's a good bet to default to the faster option, since we're talking about it, but I probably wouldn't have put all of that info on Bill if it hadn't been brought up already.

bpiel · 2025-02-19T20:11:23Z

@frenchy64 You got your wish!

frenchy64 self-requested a review February 13, 2025 19:09

frenchy64 suggested changes Feb 13, 2025

View reviewed changes

compiler+runtime/src/cpp/jank/runtime/core/seq.cpp Outdated Show resolved Hide resolved

frenchy64 self-requested a review February 13, 2025 20:03

frenchy64 reviewed Feb 13, 2025

View reviewed changes

jeaye requested changes Feb 13, 2025

View reviewed changes

bpiel force-pushed the str-nil branch 2 times, most recently from a03b7cf to 7508ea3 Compare February 19, 2025 19:03

frenchy64 reviewed Feb 19, 2025

View reviewed changes

frenchy64 approved these changes Feb 19, 2025

View reviewed changes

bpiel added 6 commits February 20, 2025 08:18

fix: (str nil) => empty string

46c0ed3

cleanup

1cbbf60

remove condition; cleanup

e3bef44

format

63e2b9a

move code out of visit_seqable lambda

5fabbf1

pass 'buff' to visit_seqable lambda

83b0785

bpiel force-pushed the str-nil branch from b41650f to 83b0785 Compare February 20, 2025 13:18

specify arg type

54df53a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (str nil) => "" #264

fix: (str nil) => "" #264

bpiel commented Feb 13, 2025 •

edited

Loading

frenchy64 commented Feb 13, 2025

frenchy64 commented Feb 13, 2025

bpiel commented Feb 13, 2025 •

edited

Loading

jeaye commented Feb 13, 2025

bpiel commented Feb 13, 2025

jeaye commented Feb 13, 2025

frenchy64 Feb 13, 2025

jeaye Feb 13, 2025

jeaye left a comment

jeaye Feb 13, 2025

bpiel Feb 19, 2025

frenchy64 Feb 19, 2025 •

edited

Loading

frenchy64 Feb 19, 2025

bpiel Feb 19, 2025

frenchy64 Feb 19, 2025

jeaye Feb 20, 2025

bpiel commented Feb 19, 2025

	return visit_object(
	[](auto const typed_l, auto const r) -> native_integer {
	using L = typename decltype(typed_l)::value_type;
	if constexpr(behavior::comparable<L>)
	{
	return typed_l->compare(*r);
	}
	else
	{
	throw std::runtime_error{ fmt::format("not comparable: {}", typed_l->to_string()) };
	}
	},
	l,
	r);

fix: (str nil) => "" #264

Are you sure you want to change the base?

fix: (str nil) => "" #264

Conversation

bpiel commented Feb 13, 2025 • edited Loading

frenchy64 commented Feb 13, 2025

frenchy64 commented Feb 13, 2025

bpiel commented Feb 13, 2025 • edited Loading

jeaye commented Feb 13, 2025

bpiel commented Feb 13, 2025

jeaye commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeaye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenchy64 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpiel commented Feb 19, 2025

bpiel commented Feb 13, 2025 •

edited

Loading

bpiel commented Feb 13, 2025 •

edited

Loading

frenchy64 Feb 19, 2025 •

edited

Loading