[Rpm-maint] [rpm-software-management/rpm] Convert variable length arrays to C++ strings and vectors (PR #4113)

[Michal Domonkos]

@dmnks approved this pull request.

Nice refactoring (with nice side effects, except those that are unintended, which hopefully are none, :knocks-wood: 😄 )!

Some comments inline, but consider them non-blocking.

> @@ -18,21 +20,16 @@ int rpmvercmp(const char * a, const char * b)
     /* easy comparison to see if versions are identical */
     if (rstreq(a, b)) return 0;
 
-    char oldch1, oldch2;
-    char abuf[strlen(a)+1], bbuf[strlen(b)+1];
-    char *str1 = abuf, *str2 = bbuf;
-    char * one, * two;
+    std::string abuf(a);
+    std::string bbuf(b);
+    char *str1 = abuf.data(), *str2 = bbuf.data();

Cosmetic: Isn't `.data()` [equivalent](https://cplusplus.com/reference/string/string/data/) to `.c_str()`? We've been using the latter throughout the codebase...

That said, I do like the name `.data()` more, for its simplicity and ease of typing 😅 It's also consistent with what we now use with e.g. the integer vector in `rpmfiArchiveReadToFilePsm()`.

> @@ -809,7 +809,7 @@ static rpmRC dbiFindByLabelArch(rpmdb db, dbiIndex dbi,
 	goto exit;
     }
 
-    *s = '\0';
+    argstr.erase(s-localarg);

Cosmetic: This could use spaces around the operator 😅 

>  
 	/* Parse for "tag=pattern" args. */
-	if ((ae = strchr(a, '=')) != NULL) {
-	    *ae++ = '\0';
-	    tag = rpmTagGetValue(a);
+	size_t sep = pat.find_first_of('=');
+	if (sep != pat.npos) {
+	    auto tagname = pat.substr(0, sep);

Turns out we don't have any tests for `rpm -qa name=foo`! 😆 I did check manually that this still works (just with a simple query, that is) but maybe we should have a test for this. Probably not worth adding in this PR so I guess I'll create a ticket.

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

Message ID: <rpm-software-management/rpm/pull/4113/review/3847149460 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20260224/2c2f834d/attachment.htm>