On Thu, Jan 10, 2019 at 04:57:47PM +0000, Kyrill Tkachov wrote: > --- a/gcc/config/aarch64/driver-aarch64.c > +++ b/gcc/config/aarch64/driver-aarch64.c > @@ -253,6 +253,12 @@ host_detect_local_cpu (int argc, const char **argv) > char *p = NULL; > char *feat_string > = concat (aarch64_extensions[i].feat_string, NULL); > + > + /* If the feature contains no HWCAPS string then ignore it for the > + auto detection. */ > + if (strlen (feat_string) == 0) > + continue; > > I think this can avoid a strlen call by checking (*feat_string == '\0') > though I believe most compilers will optimise it that way anyway. > It might be more immediately readable your way. > I wouldn't let it hold off this patch.
Well, that isn't the only problem of this code. Another one is that concat (str, NULL) is much better written as xstrdup (str); Another one is that the if (*feat_string == '\0') check should be probably if (aarch64_extensions[i].feat_string[0] == '\0') before the xstrdup, because there is no point to allocate the memory in that case. Another one is that it leaks the feat_string (right now always, otherwise if it isn't empty). Another one, I wonder if the xstrdup + strtok isn't a too heavy hammer for something so simple, especially when you have complete control over those feature strings. They use exactly one space to separate. So just do const char *p = aarch64_extensions[i].feat_string; bool enabled = true; /* If the feature contains no HWCAPS string then ignore it for the auto detection. */ if (*p == '\0') continue; size_t len = strlen (buf); do { const char *end = strchr (p, ' '); if (end == NULL) end = strchr (p, '\0'); if (memmem (buf, len, p, end - p) == NULL) { /* Failed to match this token. Turn off the features we'd otherwise enable. */ enabled = false; break; } if (*end == '\0') break; p = end + 1; } while (1); if (enabled) extension_flags |= aarch64_extensions[i].flag; else extension_flags &= ~(aarch64_extensions[i].flag); ? Last thing, for not_found, there is: return "";, why not return NULL; ? That is what other detect cpu routines do, returning "" means a confusion between when a heap allocated string is returned that needs freeing and when a .rodata string is returned which doesn't. Jakub