[Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)

[pavlinamv]

pavlinamv commented on this pull request.



>      lbuf = spec->lbuf;
     SKIPSPACE(lbuf);
     if (lbuf[0] == '#')
 	isComment = 1;
 
-     /* Don't expand macros (eg. %define) in false branch of %if clause */
-    if (!spec->readStack->reading)
+    /* Don't expand macros after %elif (resp. %elifarch, %elifos) in false branch */
+    if (ISMACROWITHARG(lbuf, "%elif")) {
+	if (!spec->readStack->readable)

The need to test for `ISMACROWITHARG(lbuf, "%elif")` in that place is caused by the current implementation of the parser and not by tracking by "reading" and "readable".

Parsing of a line in spec file consists of these for us important actions (in this order):
 1) storing the line from file into buffer  
    (function `copyNextLineFromOFI`)
 2) deciding whether the line should be expanded  
    (beginning of function `expandMacrosInSpecBuf`)
 3) if it should be expanded then expansion of the line  
    (second part of function `expandMacrosInSpecBuf`)
 4) checking if the line starts with a rpm conditional and according of it doing appropriate actions  
    (second part of function `readLine`)

Before adding `%elif`, the decision 2) whether the line should be expanded depends only on previous lines of the spec file. But after adding `%elif` it depends on the new line stored in the buffer, too. Because if the line starts with `%elif` then `if !readable` line must not be expanded, otherwise it must be expanded.

If there should be a variable that is used for deciding whether the new line should be expanded or not, then the variable must be set after 1). So it can be set 
a) in the second part of function `copyNextLineFromOFI` before calling `expandMacrosInSpecBuf`, or
b) in the first part of `expandMacrosInSpecBuf`. 
In the current patch the test is done in position b) and instead of setting a variable and then deciding according to it, the test is straightforwardly used. Thus I do not see any improvement in changing tracking variables to allow decision 2). 

To be precise I should add into `expandMacrosInSpecBuf` tests for `ISMACROWITHARG(s, "%elifarch")` and `ISMACROWITHARG(s, "%elifos")` too. And to be extremely precise with solve all corner cases I should add `ISMACRO(s, "%endif")` and `ISMACRO(s, "%else")` too. Because they all can change the potential to expand in that line. ( But usually only after `%elif` is expected to be a macro).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/613#discussion_r246021478
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20190108/1a1fa973/attachment.html>