Skip to content

js_parse_* functions should use PF_ flags, not booleans #864

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

Closed
bnoordhuis opened this issue Jan 28, 2025 · 1 comment · Fixed by #867
Closed

js_parse_* functions should use PF_ flags, not booleans #864

bnoordhuis opened this issue Jan 28, 2025 · 1 comment · Fixed by #867
Labels
enhancement New feature or request

Comments

@bnoordhuis
Copy link
Contributor

General pattern, specific example:

if (js_parse_var(s, true, tok, true))
    goto fail;

Definition:

static __exception int js_parse_var(JSParseState *s, int parse_flags, int tok,
                                    bool export_flag);

Ergo, the call site should look like this:

if (js_parse_var(s, PF_IN_ACCEPTED, tok, /*export_flag*/true))
    goto fail;

It works okay because PF_IN_ACCEPTED == 1 and true == 1 but it's confusing.

In the particular case of js_parse_var, that export_flag boolean could maybe be a PF_ flag instead. On the other hand, you could argue module exports aren't a parser concern and I wouldn't disagree.

@bnoordhuis bnoordhuis added the enhancement New feature or request label Jan 28, 2025
@saghul
Copy link
Contributor

saghul commented Jan 28, 2025

I'll take a look after I get my head out of this printf hole I've dug for myself 😅

Re: the export flag. There are more parsing functions that do that and I'm afraid a significant refactor would be necessary to detach the parser from handling the exports.

bnoordhuis added a commit to bnoordhuis/quickjs that referenced this issue Jan 31, 2025
Declaring int when the parameter is in fact used as a bool is needlessly
confusing.

Fixes: quickjs-ng#864
bnoordhuis added a commit that referenced this issue Jan 31, 2025
Declaring int when the parameter is in fact used as a bool is needlessly
confusing.

Fixes: #864
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants