On Mon Feb 13 22:02:11 2023 +0000, Bernhard Kölbl wrote:
These don't need to be inline, do they?
Why not.
vosk uses vosk_ as a namespace prefix, so it seems that for
readability it would be better to avoid anything beginning with that name in our code. That includes other cases in this file, e.g. "vosk_handle", "vosk_model_identifier". ok
Do you need these checks?
Yes, yes, maybe.
Is there some legitimate scenario where this can fail?
Yes, if the model path isn't created.
(And if not, shouldn't we print an ERR?) Is it worth pulling in
errno_to_status() for this? Sure, I can print an ERR. I pulled in that function for possible future uses.
Isn't the ".." check redundant here?
Not sure what you mean? It's a folder that's always listed so we skip it?
Does Vosk really expect every API consumer to do this all manually?
This looks extremely suspicious. Sort of, yes. They have a Python wrapper for this, but obviously I don't wanna interface with Python scripts.
Also, looking at this function, this looks like a very odd way to
search for models. What's expected to follow the language and region, and why don't we care about it? Or, if the answer is "nothing", why are we doing an entire readdir() loop instead of just trying the 4 different possibilities? There is something after the lang and region, but we indeed don't care about it because that's exactly how the Python script does it as well.
It seems mildly clearer to add an "else" case here rather than
checking for a previous initialized variable below. Sorry, I don't understand this one.
This "error" label is only used in one place and only has one line of
cleanup, making it look a little redundant. I also believe that "model" cannot be NULL in this case, can it? Yes, I can remove it.
Please don't strip out the code I'm commenting on, that makes this reply hard to read.
These don't need to be inline, do they?
Why not.
Why do they need to be inline?
Do you need these checks?
Yes, yes, maybe.
Why do you need those checks?
Is there some legitimate scenario where this can fail?
Yes, if the model path isn't created.
What do you mean by this?
Isn't the ".." check redundant here?
Not sure what you mean? It's a folder that's always listed so we skip it?
It's already covered by the following prefix checks, isn't it?
Does Vosk really expect every API consumer to do this all manually? This looks extremely suspicious.
Sort of, yes. They have a Python wrapper for this, but obviously I don't wanna interface with Python scripts.
So they export multiple language bindings, except they require Python in order to do certain basic library tasks like actually finding the models??
And this is all part of stable API??
Can we maybe get a function into their C API to do this for us? This just feels very poorly designed, and not the kind of thing that Wine should have to do.
It seems mildly clearer to add an "else" case here rather than checking for a previous initialized variable below.
Sorry, I don't understand this one.
I mean something like:
if ((env = getenv("XDG_CACHE_HOME"))) suffix = "/vosk"; else if ((env = getenv("HOME"))) suffix = "/.cache/vosk"; else return STATUS_UNSUCCESSFUL;
(possibly with some ERR in there, because undefined $HOME seems like a catastrophic failure that the user should know about, but anyway)
That way you don't need to initialize the "suffix" variable.