Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
Superceded by @mlschroe 's work to add ternary operator to expression parsing and wiring all that into the macro engine, closing. Thanks @pavlinamv for your initiative on this though, I do think it was vital for getting the ball rolling on the whole thing! -- 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/746#issuecomment-534043724___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
Closed #746. -- 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/746#event-2653949216___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
As mentioned in the ticket (#115), I want to see a plan for a syntax that allows for generic condition instead of just existence test before proceeding. Also mentioned in the ticket, the syntax has other issues too. Lets discuss those in the ticket to keep it all in one place. @pavlinamv , while it's perfectly fine to remove "blocked" flag set by yourself, leave it alone when others set it. It's a warning flag to others to not merge even if other indicators appear in the green - for example here we're only discussing the syntax being introduced, and we're not ready for merge even if the code were technically acceptable and passing tests. Finally, this has conflicts now and should be rebased, but that can wait until the syntax side is settled. -- 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/746#issuecomment-520325143___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
Support for extra spaces is ripped out. -- 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/746#issuecomment-518617194___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai requested changes on this pull request. If I interpret that correctly, you just dismiss all the spaces that might be there. So what if you WANT to emit spaces? Also, the triple syntax has to give the same exact results as doing the same thing without the older conditional operator for consistency's and sanity's sake. If I interpret that correctly, you just dismiss all the spaces that might be there. So what if you WANT to emit spaces? It's equally common and legit thing to do. Also, the triple syntax has to give the same exact results as doing the same thing without the older conditional operator for consistency's and sanity's sake. Rip out support for all those extra spaces, I'm not going to accept this as long as they're there. -- 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/746#pullrequestreview-271125720___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
> And how do you're supposed to know which spaces before and after the 0/1 are
> intentional or not? This must expand literally to either " 0 " or " 1".
According to the specification of triple operator - all spaces
- after '%{?!' or '%{?',
- before and after ':' that divides the operator and
- before the last '}'
till the first nonspace char must been omitted. This can be described in the
commit message explicitly.
Packagers tend to use spaces before ':' in the current condition operator, even
if "courtesy"
spaces are not supported. In the current case spaces usually do not change the
meaning of the spec file line and improve readability and alignment.
--
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/746#issuecomment-518247611___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
```
%global with_lua %{?{_without_lua} : 0 : 1}
```
And how do you're supposed to know which spaces before and after the 0/1 are
intentional or not? This *must* expand literally to either " 0 " or " 1".
Rip support for the "courtesy" spaces everywhere.
--
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/746#issuecomment-518211001___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
> The exact syntax is subject to endless bikeshedding of course, but one thing
> that strikes me as just wrong are the surrounding spaces everywhere. There
> are no "courtesy spaces" for readability anywhere in rpm macros, I dont think
> this should be any different.
Here are 2 relevant examples from the current spec files (the most frequently
used):
```%global with_lua %{?_without_lua: 0} %{?!_without_lua: 1}```
```%{?gitdate:%{gitdate}}%{?!gitdate:%{version}}```
Without "courtesy spaces" the whole triple conditional macro looks:
```%global with_lua %{?{_without_lua}:0:1}```
```%{?{gitdate}:%{gitdate}:%{version}}```
But with spaces it looks better:
```%global with_lua %{?{_without_lua} : 0 : 1}```
```%{?{gitdate} : %{gitdate} : %{version}}```
Note that inside conditionally expanded macros packagers tend to use spaces
after ':' even if there are not supported. Thus it looks that packagers prefer
to use spaces that help to have the macro readable.
> This also makes me wonder if it's really worth the added complexity, it's
> merely syntactic sugar afterall.
It is a syntactic sugar, but it will help to improve packager experience with
spec files.
> Any particular reason for this specific syntax over the others discussed in
> the ticket?
I asked several packagers and this was the preferred syntax. FYI remaining
possibilities are
```%{? {_without_lua} : 0 ! 1 }```
```%{? {_without_lua} ? 0 ! 1 }```
```%{? {_without_lua} ? 0 : 1 }```
```%{?: _without_lua : 0 : 1 }```
```%{? _without_lua ? 0 : 1 }```
> Oh and to make it clear, this is strictly 4.16 material, we don't want
> changes this drastic at this point in 4.15 cycle, so there's all the time in
> the world for review and further discussion.
I have a deadline - 4th October.
--
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/746#issuecomment-509658688___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
The exact syntax is subject to endless bikeshedding of course, but one thing that strikes me as just wrong are the surrounding spaces everywhere. There are no "courtesy spaces" for readability anywhere in rpm macros, I dont think this should be any different. This also makes me wonder if it's *really* worth the added complexity, it's merely syntactic sugar afterall. Any particular reason for this specific syntax over the others discussed in the ticket? Oh and to make it clear, this is strictly 4.16 material, we don't want changes this drastic at this point in 4.15 cycle, so there's all the time in the world for review and further discussion. -- 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/746#issuecomment-503987339___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
> It is a small functional change. But the change is unimportant,
because it includes only some of %load macros with more
than one '?'. Like:
%{!??load:file}
%{!???load:file}
or
%{!!!??load:file}
All this talk about a change, but it fails to explain what that change of
behavior actually *is*.
I'd say all of the above should be just errors.
--
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/746#issuecomment-503969355___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> str++;
}
return str;
}
+static void partsInit(macroPartition *parts)
+{
+parts->f = parts->fe = NULL;
+parts->g = parts->ge = NULL;
+parts->h = parts->he = NULL;
+parts->fn = parts->gn = parts->hn = 0;
+return;
In general, never initialize more than one variable on a line, it only makes it
harder to read. In a case like this, just memset() to zero.
The return at end of void function is redundant.
--
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/746#pullrequestreview-252203689___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> @@ -45,6 +45,12 @@ enum macroFlags_e {
ME_USED= (1 << 1),
};
+enum checkConditionType {
+CHK_NO = 0,
+CHK_BASIC = (1 << 0),
+CHK_TRIPLE = (1 << 1),
+};
CHK_NO sounds like something that wants to pair with CHK_YES, but this is
something entirely different.
I didn't really look whether it makes sense in the rest of the code, I'd expect
the to be a CHK_EXISTS bit, and if its of the triple format, there'd be an
extra bit set on that, instead of being entirely separate entities. But please
rethink these names anyhow.
--
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/746#pullrequestreview-252202908___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> +return;
+}
+
+static void setPartsSize(macroPartition *parts)
+{
+parts->fn = parts->fe - parts->f;
+parts->gn = parts->ge - parts->g;
+parts->hn = parts->he - parts->h;
+return;
+}
+
+/**
+ * Skip spaces before the input string
+ * @param str string that starts after the ..
+ * @
+ */
In general, doxygen annotations don't belong to internal helper functions,
trivial in particular.
--
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/746#pullrequestreview-252200956___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> +SKIPBLANK(parts->g, c);
+
+/* after %{? {macroo} must be ':' */
+if (parts->g[0] == ':') {
+ parts->g++;
+ SKIPBLANK(parts->g,c);
+ parts->ge = parts->g;
+ while ((parts->ge[0] != 0) && (parts->ge[0] != ':')) {
+ parts->ge++;
+ if (parts->ge[0] == '{') {
+ if ((parts->ge = matchchar(parts->ge++, '{', '}')) == NULL)
+ return NULL;
+ }
+ }
+} else {
+ return NULL;
Arrange a single point of return to the function, goto exit or such.
--
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/746#pullrequestreview-252200124___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> +if (parts->g[0] == ':') {
+ parts->g++;
+ SKIPBLANK(parts->g,c);
+ parts->ge = parts->g;
+ while ((parts->ge[0] != 0) && (parts->ge[0] != ':')) {
+ parts->ge++;
+ if (parts->ge[0] == '{') {
+ if ((parts->ge = matchchar(parts->ge++, '{', '}')) == NULL)
+ return NULL;
+ }
+ }
+} else {
+ return NULL;
+}
+
+/* be the trhird part starting by ':' is optional */
The comment has typos and doesn't make sense.
--
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/746#pullrequestreview-252199480___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support triple operator for conditional shortcut (#115) (#746)
pmatilai commented on this pull request.
> + SKIPBLANK(parts->g,c);
+ parts->ge = parts->g;
+ while ((parts->ge[0] != 0) && (parts->ge[0] != ':')) {
+ parts->ge++;
+ if (parts->ge[0] == '{') {
+ if ((parts->ge = matchchar(parts->ge++, '{', '}')) == NULL)
+ return NULL;
+ }
+ }
+} else {
+ return NULL;
+}
+
+/* be the trhird part starting by ':' is optional */
+parts->h = parts->ge;
+parts->ge = findParameterEnd(parts->h[0] == ':' ? parts->ge : --parts->ge);
Never use --/++ side-effects like that. You're even assigning to the same
variable so it should be just
```? parts->ge : parts->ge -1```
--
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/746#pullrequestreview-252199254___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
