[Rpm-maint] [rpm-software-management/rpm] Add support for defining literal macros via a new modifier syntax (PR #4129)

[Michal Domonkos]

@dmnks commented on this pull request.

Apart from the below nits, looks good to me.

Having said that, I'm having second thoughts about the `<>` syntax 😅 Somehow it's just so... ugly and convoluted 😆  The colon-separated modifier list looks so much cleaner...

There's also a little practicality issue I've noticed when writing the below comments: The `<` and `>` symbols are interpreted in GH markdown and hidden, so if you type e.g. "%mymacro\<l\>" literally in a GH comment, it'll render as "%mymacro". Ideally, one always uses code blocks of course, and taking into account a (GitHub) specific quirk when designing such an RPM feature sounds totally crazy of course, so there's that.

On the other hand, the `<>` syntax is more consistent with the options (and possibly arrays in the future), so that's good...

> @@ -738,6 +748,19 @@ Expands to *1* if *%aa* expands to *5*, otherwise expands to *2*:
 %[ "%{aa}" == "5" ? 1 : 2]
 ```
 
+## Example 7. Literal macros
+
+Define a macro whose body contains literal *%* characters, possibly
+overlapping with other macros:
+
+```
+%mymacro 1.2.3
+%aa %mymacro
+%bb<l> %mymacro

This example is minimal and clear (which is nice) but it doesn't quite demonstrate the actual usefulness of the literal macros, I feel like. How about something like:
```
%myversion 1.2.3
%mytitle Program version %myversion
%myhint<l> Program version is stored in %myversion
```

>      } else if (c == '{') {	/* XXX permit silent {...} grouping */
 	if ((se = matchchar(s, c, '}')) == NULL) {
 	    rpmMacroBufErr(mb, 1, _("Macro %%%s has unterminated body\n"), n);
 	    se = s;	/* XXX W2DO? */
 	    goto exit;
 	}
 	s++;	/* XXX skip { */
-	strncpy(b, s, (se - 1 - s));
-	b[se - 1 - s] = '\0';
-	be += strlen(b);
+	body = str.substr(s - se, (se - 1 - s));

`se` points to the closing curly brace here (not the beginning of the string as it would earlier), so `s - se` seems wrong here and should instead be `s - str.c_str()`.

Curiously, the test-suite doesn't seem to cover this `{}` macro variant at all (it passes with this commit, and doesn't fail even if I add a `printf()` line in this code block). Should we create a ticket for that?

>      if (*s == '(') {
 	s++;	/* skip ( */
 	/* Options must be terminated with ')' */
 	if (strchr(s, ')')) {
-	    o = oe;
-	    COPYOPTS(oe, s, oc);
+	    int oc = ')';
+	    COPYOPTS(opts, s, oc);
+	    o = opts.c_str();

It seems like we could just drop `o` as well here (like we did `n` and `b`). That would allow us to directly use `opts` (like the other strings) in the `pushMacro()` call below as well.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/4129#pullrequestreview-3990817305
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/4129/review/3990817305 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20260323/bb5e8b93/attachment.htm>