Re: [PATCH] fix -Wmaybe-uninitialized with -Og
On Mon, Aug 04, 2025 at 03:07:15AM -0700, Denton Liu wrote:
> When building with -Og on gcc 15.1.1, the build produces two warnings.
> Even though in practice, these codepaths can't actually be hit while the
> variables are uninitialized, satisfy the compiler by initializing the
> variables.
I see these on gcc 14.2.0, too.
Show 13 quoted lines
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5dd6cbbaee..cc462677e1 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1463,7 +1463,7 @@ static int set_head(int argc, const char **argv, const char *prefix,
> b_local_head = STRBUF_INIT;
> char *head_name = NULL;
> struct ref_store *refs = get_main_ref_store(the_repository);
> - struct remote *remote;
> + struct remote *remote = NULL;
>
> struct option options[] = {
> OPT_BOOL('a', "auto", &opt_a,I think you're right that this can't be triggered, but maybe a bit of reordering would make that more obvious both to the compiler and to humans. The issue is that we do this:
if (argc) {
strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
remote = remote_get(argv[0]);
}and then follow it up with various sanity checks about how the value of argc. But we always require that argc is at least 1 or we bail with a usage message.
This comes from 012bc566ba (remote set-head: set followRemoteHEAD to "warn" if "always", 2024-12-05). If we revert out that change and instead add it later, like so:
Show changes to builtin/remote.c +10 −8
diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..8ba02d1854 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,8 @@ static int set_head(int argc, const char **argv, const char *prefix,
};
argc = parse_options(argc, argv, prefix, options,
builtin_remote_sethead_usage, 0);
- if (argc) {
+ if (argc)
strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
- remote = remote_get(argv[0]);
- }
if (!opt_a && !opt_d && argc == 2) {
head_name = xstrdup(argv[1]);
@@ -1501,6 +1499,8 @@ static int set_head(int argc, const char **argv, const char *prefix,
} else
usage_with_options(builtin_remote_sethead_usage, options);
+ remote = remote_get(argv[0]);
+
if (!head_name)
goto cleanup;
strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name);
then the compiler is happy. Though I still find the whole b_head thing
to be total spaghetti (it must be set before our argc if/else cascade
because deletion mode expects it there, but we can't just set it inside
there because other modes expect it to be set later on).
So I wonder if this would be much more obvious (again, to both humans
and compilers):
diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..f0e49a5681 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
};
argc = parse_options(argc, argv, prefix, options,
builtin_remote_sethead_usage, 0);
- if (argc) {
- strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
- remote = remote_get(argv[0]);
- }
+
+ /* All modes require at least a remote name. */
+ if (!argc)
+ usage_with_options(builtin_remote_sethead_usage, options);
+
+ strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
+ remote = remote_get(argv[0]);
if (!opt_a && !opt_d && argc == 2) {
head_name = xstrdup(argv[1]);
> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
> index d54e455367..03a3aa8e87 100644
> --- a/t/unit-tests/clar/clar.c
> +++ b/t/unit-tests/clar/clar.c
> @@ -350,7 +350,7 @@ static void
> clar_run_suite(const struct clar_suite *suite, const char *filter)
> {
> const struct clar_func *test = suite->tests;
> - size_t i, matchlen;
> + size_t i, matchlen = 0;
> struct clar_report *report;
> int exact = 0;
For this one I don't see any real alternative. We set matchlen if and
only if "filter" is set:
if (filter) {
...
matchlen = strlen(filter);
...
}
and the line it complains about is:
if (filter && strncmp(test[i].name, filter, matchlen))
so the short-circuit protects us. So it's only a problem if "filter"
changes between those two lines. It is in a loop, but I don't see how
"filter" could ever be changed within the loop. So I'm not sure if this
is just a compiler bug, or if there's some really subtle alias thing
where filter _could_ be changed in a sub-function or something.
At any rate I agree that "0" is the appropriate value here, and
assigning it to shut up the compiler is the best approach.
-Peff
Denton Liu
