jrgemignani commented on PR #2415:
URL: https://github.com/apache/age/pull/2415#issuecomment-4331055419
@crprashant Please address these minor issues -
## Recommendation
1. **Drop the redundant `pstrdup` in the new action body.**
The `safe_keywords` rule already runs `KEYWORD_STRDUP($1)` on every
alternative, which is `pnstrdup`-backed. The extra `pstrdup((char *) $1)` in
`var_name_alias` is a second heap allocation per parse with no purpose. Replace
it with `$$ = (char *) $1;` to match the established pattern in `schema_name`:
```yacc
schema_name:
symbolic_name
| reserved_keyword
{
/* we don't need to copy it, as it already has been */
$$ = (char *) $1;
}
;
```
This is a one-line cosmetic improvement that also makes the code
symmetric with neighbouring rules and slightly reduces parser allocations on
alias-heavy queries.
2. **Add an inline comment in `var_name_alias` documenting the
unreadable-alias limitation, with an issue link.**
The PR description correctly notes that `WITH 1 AS count RETURN count`
still fails because `expr_var` reads through `var_name`, which is unchanged.
That asymmetry (writable but not readable) will surprise the next maintainer
who reads only the grammar file. Add 2–3 lines pointing to the follow-up issue,
e.g.:
```c
/*
* NOTE: Reading a keyword-named alias back (e.g. WITH 1 AS count RETURN
count)
* is intentionally still rejected — broadening expr_var to accept
safe_keywords
* reintroduces ~156 shift/reduce conflicts. Tracked in issue #NNNN.
*/
```
File the follow-up issue and reference its number here so the limitation
is discoverable from the source, not just the merged PR description.
3. **Add two regression cases to round out coverage.**
- **Backtick-quoted alias positive case:** `RETURN 1 AS \`count\`` —
confirms that quoting forces the `IDENTIFIER` token path and continues to work,
so future grammar refactors don't accidentally regress quoted identifiers.
- **Known-limitation negative case:** `WITH 1 AS count RETURN count` —
currently fails. Adding it with the failing expected output gives the next
contributor a precise file to update when they fix `expr_var`, and serves as
permanent self-documentation in the regress tree. Mark it clearly in the test
file with a comment like `-- Known limitation: see issue #NNNN`.
4. **Fix the doc nit in the `var_name_alias` comment.**
The current comment says the new rule permits "the same set of
non-conflicting reserved keywords accepted by `safe_keywords` (and already
accepted in `func_name` / `schema_name`)." That is slightly inaccurate:
`func_name` accepts `safe_keywords`, but `schema_name` accepts the broader
`reserved_keyword` (which includes `conflicted_keywords`). Tighten the wording
to "(already accepted in `func_name`; `schema_name` accepts the broader
`reserved_keyword`)" so a future reader doesn't infer that `schema_name` is a
1:1 precedent for the new rule.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
