New pcmk__compare_versions(), deprecate compare_version() #3970
Conversation
Clearly there is work to be done, even for this crude version comparison function that's designed only for dotted decimal versions. We'll create a new internal function soon and deprecate the public one. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
2 times, most recently
from
b985422 to
ad046d3
Compare
To replace compare_versions(). Seems a bit more readable and a bit less error-prone. I started working on an implementation that just uses pcmk__numeric_strcasecmp(), but it started getting ugly for things like "1" vs. "1.0", since it treats all non-digit characters equally (in particular, it doesn't treat dots specially). pcmk__compare_versions() could also reasonably go into strings.c, by the way. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Seems simpler, even if it's heavier-weight and more than what we need for this particular use. Why reinvent the wheel? Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
And update comment in nodes.h Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-refactors
branch
from
ad046d3 to
f246956
Compare
| static void | ||
| assert_compare_version(const char *v1, const char *v2, int expected_rc) | ||
| { | ||
| assert_int_equal(compare_version(v1, v2), expected_rc); |
There was a problem hiding this comment.
This isn't something for this PR, but you made me think of it. If we have our own assert_ wrappers like this, any assertion failures will be logged at this line, instead of the line that calls the wrapper.
For instance:
diff --git a/lib/common/tests/cmdline/pcmk__quote_cmdline_test.c b/lib/common/tests/cmdline/pcmk__quote_cmdline_test.c
index bd0817909e..bd6374c230 100644
--- a/lib/common/tests/cmdline/pcmk__quote_cmdline_test.c
+++ b/lib/common/tests/cmdline/pcmk__quote_cmdline_test.c
@@ -39,7 +39,7 @@ static void
no_spaces(void **state)
{
const char *argv[] = {
- "crm_resource", "-r", "rsc1", "--meta", "-p", "comment",
+ "crm_resource", "-x", "rsc1", "--meta", "-p", "comment",
"-v", "hello", "--output-as=xml", NULL,
};
$ make check
...
FAIL: pcmk__quote_cmdline_test 2 - no_spaces
...
$ cat test-suite.log
...
not ok 2 - no_spaces
FAIL: pcmk__quote_cmdline_test 2 - no_spaces
# pcmk__str_any_of(processed, expected_single, expected_double, NULL)
# pcmk__quote_cmdline_test.c:27: error: Failure!
Where line 27 is the assert_true in assert_quote_cmdline. If the caller is in a function with a lot of other callers, it's more work to figure it out. Again, definitely not something to address in this PR. We have a lot of assert wrappers and I'm sure I wrote a bunch of them. It's just something to look into in the future.
| * \note Each version segment's parsed value must fit into a <tt>long long</tt>. | ||
| */ | ||
| int | ||
| pcmk__compare_versions(const char *version1, const char *version2) |
There was a problem hiding this comment.
I'm not really happy with treating an invalid argument as essentially NULL or "" and then returning a value based on comparing against that. I'd prefer EINVAL or something. I'm not sure we could really do that without making the callers a lot more complicated. I'm also not sure it really matters - most of the callers seem like they're comparing protocol versions (which we're responsible for as long as there's no network problems) or CIB versions (which I expected to be more strictly defined, but they're actually just text).
| * \note Each version segment's parsed value must fit into a <tt>long long</tt>. | ||
| */ | ||
| int | ||
| pcmk__compare_versions(const char *version1, const char *version2) |
There was a problem hiding this comment.
Speaking of putting pcmk__compare_versions into strings.c, a particularly crazy idea I had was that it could just all be part of pcmk__strcmp and we introduce a new pcmk__str_version flag. Looking into that led me to discovering pcmk__cmp_by_type, pcmk__type, and pcmk__comparison. I don't have much of a point here, aside from noting that we sure do have a lot of overlapping stuff in this area.
| assert_compare_version("1.0", "1.0.1", -1); | ||
| assert_compare_version("1.0", "1", 0); | ||
| assert_compare_version("1", "1.2", -1); | ||
| assert_compare_version("1.0.0", "1.0", 0); |
There was a problem hiding this comment.
Not that we need to follow this exactly, but I think it's worth pointing out that RPM would treat "1.0.0" as newer than "1.0" because it has more segments (https://fedoraproject.org/wiki/Archive:Tools/RPM/VersionComparison#The_rpmvercmp_algorithm)
2 participants
This is the next batch from the refactors I've had on the shelf for a long time. I ended up changing the approach a little bit compared to my saved commits, and that took a while to get it to a place where it seemed sufficiently clear. (For example, I tried doing the entire split using only a GRegex and no
g_strsplit(). The code worked well and was concise, but it required a comment that was about 10 lines long to explain it to a person not well-versed in PCREs. So I went back to the drawing board.)