On Tue, 18 Jun 2024, Jakub Jelinek wrote:
> The following patch implements the C23 N3017 "#embed - a scannable,
> tooling-friendly binary resource inclusion mechanism" paper.
Some initial comments, not yet reviewed the whole patch.
> The patch uses --embed-dir= option that clang plans to add above and doesn't
> use other variants on the search directories yet, plus there are no
> default directories at least for the time being where to search for embed
> files. So, #embed "..." works if it is found in the same directory (or
> relative to the current file's directory) and #embed "/..." or #embed </...>
> work always, but relative #embed <...> doesn't unless at least one
> --embed-dir= is specified. There is no reason to differentiate between
> system and non-system directories, so we don't need -isystem like
> counterpart, perhaps -iquote like counterpart could be useful in the future,
> dunno what else. Should we have --embed-directory= as alias to --embed-dir=?
Having that alias seems reasonable and consistent with various other
options.
-Wmissing-include-dirs should probably warn about missing embed
directories (I don't think a separate -Wmissing-embed-dirs is needed). I
suspect that already works with this patch, but a testcase should be
included.
> +/* Skip over balanced preprocessing tokens until END is found.
> + If SAVE is non-NULL, remember the parsed tokens in it. */
Document the parameter NESTED.
> +/* Parse parameters of #embed directive or __has_embed expression. */
> +
> +bool
> +_cpp_parse_embed_params (cpp_reader *pfile, struct cpp_embed_params *params)
Document the meaning of the return value.
> + else if (param_kind == 0)
> + {
> + if (params->has_embed && pfile->op_stack == NULL)
> + _cpp_expand_op_stack (pfile);
> + params->limit = _cpp_parse_expr (pfile, "#embed", token);
> + token = _cpp_get_token_no_padding (pfile);
> + }
> + else if (token->type == CPP_OPEN_PAREN)
> + {
> + cpp_embed_params_tokens *save = NULL;
> + auto save_comments = pfile->state.save_comments;
> + switch (param_kind)
> + {
> + case 1: save = ¶ms->prefix; break;
> + case 2: save = ¶ms->suffix; break;
> + case 3: save = ¶ms->if_empty; break;
Comparing param_kind against constants 0, 1, 2, 3 isn't very readable (and
it only gets worse in subsequent patches in the series comparing with 4
and 5 as well). I think there should be actual defined constants (whether
enumeration constants or macros) indexing the table(s) of embed
parameters, rather than hardcoding 0 through 5. (Or some system with
function pointers for embed parameter handlers in the table, so that such
constants aren't needed.)
--
Joseph S. Myers
josmy...@redhat.com
