Skip to content

Conversation

ppotapov-aws
Copy link
Collaborator

@ppotapov-aws ppotapov-aws commented Sep 4, 2025

Implement FromNKI? functionality defined in lean using c++
Notes:

  • We don't have a tensor library. I will defer until later with tensor implementation
  • The access definition doesn't match lean definition. I can follow up here or on separate PR

Copy link
Collaborator

@graebm graebm left a comment

Choose a reason for hiding this comment

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

I stopped an 1/8 of the way in to this review. But here's what I have

Comment on lines +84 to +87
struct Builtin, struct Ref, struct Fun,
struct Var,
std::monostate, // used with None, Elipsis, Mgrid
bool, int, float, struct Slice, klr::Address,
Copy link
Collaborator

Choose a reason for hiding this comment

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

utterly trivial: don't need struct in these lines

In .c files, you need to put struct everywhere you use custom types. But in C++ it's unnecessary.


struct Error {
std::string msg;
Error(std::string&& m) : msg(std::move(m)) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you just take the string by value, it's way easier for users (and just as efficient). They can call it like: Error("oh no") instead of Error(std::move(std::string("oh no"))). I see you discovered that and made a TraceHelper(std::string msg) helper function later to get the same result as what I'm suggesting here. You wouldn't need that helper anymore

The only advantage of the "r-value only" version you made here is that users are forced to call move() themselves and can't accidentally do a copy instead. But you can see the downside, as you had to make a helper function to work around the clunkiness.

Suggested change
Error(std::string&& m) : msg(std::move(m)) {}
Error(std::string m) : msg(std::move(m)) {}

Comment on lines +30 to +34

Error TraceError(std::string msg) {
return Error{std::move(msg)};
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this if you make change to Error constructor I suggested above

Suggested change
Error TraceError(std::string msg) {
return Error{std::move(msg)};
}


// Simple error handling
template<typename T>
using TraceResult = std::variant<T, Error>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need it in this PR, but I'd love a proper Result class.
Or use c++ exceptions.

Comment on lines +27 to +28
T get_ok(const TraceResult<T>& result) {
return std::get<T>(result);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is returning a copy of T. Because it takes result by reference, but return a T by value.

We might want it to be more like std::get for variant and std::expected::value(). Returning either a reference, or rvalue, to what's inside

template<typename T>
T& get_ok(const TraceResult<T>& result) {
    return std::get<T>(result);
}

template<typename T>
T&& get_ok(TraceResult<T>&& result) {
    return std::get<T>(std::move(result));
}

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