grammar: fix regression caused by #17381 #17412
Conversation
pwilkin
left a comment
pwilkin
left a comment
There was a problem hiding this comment.
Yeah, my bad, should've used a system-independent data type to begin with (but see comment on condition).
src/llama-grammar.cpp
Outdated
| throw std::runtime_error(std::string("expecting ',' at ") + pos); | ||
| } | ||
| if (min_times > MAX_REPETITION_THRESHOLD || (max_times != UINT64_MAX && max_times > MAX_REPETITION_THRESHOLD)) { | ||
| if (min_times > MAX_REPETITION_THRESHOLD || max_times > MAX_REPETITION_THRESHOLD) { |
There was a problem hiding this comment.
This is going to be wrong. The condition was "if min_times exceeds threshold or (max_times is defined and exceeds threshold)". Now, this is going to trigger if max_times is not defined (so with X{n,} patterns)
There was a problem hiding this comment.
If we're not going to use std::optional like I wanted in the first approach, then I think this is the only way to do it. This is bascially equivalent to checking for == -1 or, as the original test had it, < 0.
There was a problem hiding this comment.
hmm ok I thought one of the condition was overlapping the other.
using uint_max as a special value is fine, the main advantage of std::optional is its readability which is easy to replicate: c0b9903
|
LGTM now. |
|
(I can't merge BTW so you'll have to get someone with write access to approve a review) |
|
For extra safe, I'll merge when the windows CI on my fork passes (skipping long waiting line on the main repo) |
Ref: #17381 (comment)
uint64_tto align with the rest of the code base