From patchwork Tue Feb 18 12:07:01 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Liebler X-Patchwork-Id: 38200 X-Patchwork-Delegate: fweimer@redhat.com Received: (qmail 28029 invoked by alias); 18 Feb 2020 12:07:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 28021 invoked by uid 89); 18 Feb 2020 12:07:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx0b-001b2d01.pphosted.com Subject: Re: [PATCH] Move implementation of into a C file To: libc-alpha@sourceware.org References: <87tv3pqwpr.fsf@oldenburg2.str.redhat.com> From: Stefan Liebler Date: Tue, 18 Feb 2020 13:07:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <87tv3pqwpr.fsf@oldenburg2.str.redhat.com> x-cbid: 20021812-0028-0000-0000-000003DC1911 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 20021812-0029-0000-0000-000024A121D4 Message-Id: Hi Florian, I've run some builds on my s390x and x86_64 machines with your applied patch. I do not see the -Werror=maybe-uninitialized anymore. Despite of the whitespace issues, this patch looks good to me. At least the second line of arguments for __file_is_unchanged needs two more spaces (see below). I've also opened include/file_change_detection.h and io/file_change_detection.c in my emacs and it showed more whitespace errors: tab vs whitespaces. Please have a look at the attached patch to see what I mean. I'm not exactly sure if this is really required. If yes, please also check the remaining files in the patch. On 2/17/20 12:28 PM, Florian Weimer wrote: > file_change_detection_for_stat partially initialize > struct file_change_detection in some cases, when the size member > alone determines the outcome of all comparisons. This results > in maybe-uninitialized compiler warnings in case of sufficiently > aggressive inlining. > > Once the implementation is moved into a separate C file, this kind > of inlining is no longer possible, so the compiler warnings are gone. > > (Note that I could not reproduce this warning in my tests.) > > ----- > include/file_change_detection.h | 110 +++++++--------------------------- > io/Makefile | 2 +- > io/Versions | 4 ++ > io/file_change_detection.c | 129 ++++++++++++++++++++++++++++++++++++++++ > io/tst-file_change_detection.c | 57 +++++++++--------- > resolv/res_init.c | 2 +- > resolv/resolv_conf.c | 6 +- > 7 files changed, 185 insertions(+), 125 deletions(-) > > diff --git a/include/file_change_detection.h b/include/file_change_detection.h > index aaed0a9b6d..f5220745a9 100644 > --- a/include/file_change_detection.h > +++ b/include/file_change_detection.h > @@ -16,9 +16,10 @@ > License along with the GNU C Library; if not, see > . */ > > -#include > +#ifndef _FILE_CHANGE_DETECTION_H > +#define _FILE_CHANGE_DETECTION_H > + > #include > -#include > #include > #include > #include > @@ -38,103 +39,32 @@ struct file_change_detection > > /* Returns true if *LEFT and *RIGHT describe the same version of the > same file. */ > -static bool __attribute__ ((unused)) > -file_is_unchanged (const struct file_change_detection *left, > - const struct file_change_detection *right) > -{ > - if (left->size < 0 || right->size < 0) > - /* Negative sizes are used as markers and never match. */ > - return false; > - else if (left->size == 0 && right->size == 0) > - /* Both files are empty or do not exist, so they have the same > - content, no matter what the other fields indicate. */ > - return true; > - else > - return left->size == right->size > - && left->ino == right->ino > - && left->mtime.tv_sec == right->mtime.tv_sec > - && left->mtime.tv_nsec == right->mtime.tv_nsec > - && left->ctime.tv_sec == right->ctime.tv_sec > - && left->ctime.tv_nsec == right->ctime.tv_nsec; > -} > +bool __file_is_unchanged (const struct file_change_detection *left, > + const struct file_change_detection *right); Here we need two more spaces before const. Thanks, Stefan diff --git a/include/file_change_detection.h b/include/file_change_detection.h index f5220745a9..0581d2f584 100644 --- a/include/file_change_detection.h +++ b/include/file_change_detection.h @@ -40,25 +40,25 @@ struct file_change_detection /* Returns true if *LEFT and *RIGHT describe the same version of the same file. */ bool __file_is_unchanged (const struct file_change_detection *left, - const struct file_change_detection *right); + const struct file_change_detection *right); /* Extract file change information to *FILE from the stat buffer *ST. */ void __file_change_detection_for_stat (struct file_change_detection *file, - const struct stat64 *st); + const struct stat64 *st); /* Writes file change information for PATH to *FILE. Returns true on success. For benign errors, *FILE is cleared, and true is returned. For errors indicating resource outages and the like, false is returned. */ bool __file_change_detection_for_path (struct file_change_detection *file, - const char *path); + const char *path); /* Writes file change information for the stream FP to *FILE. Returns ture on success, false on failure. If FP is NULL, treat the file as non-existing. */ bool __file_change_detection_for_fp (struct file_change_detection *file, - FILE *fp); + FILE *fp); #ifndef _ISOMAC libc_hidden_proto (__file_is_unchanged) diff --git a/io/file_change_detection.c b/io/file_change_detection.c index 1ac869c3ec..fcc0705bff 100644 --- a/io/file_change_detection.c +++ b/io/file_change_detection.c @@ -25,7 +25,7 @@ same file. */ bool __file_is_unchanged (const struct file_change_detection *left, - const struct file_change_detection *right) + const struct file_change_detection *right) { if (left->size < 0 || right->size < 0) /* Negative sizes are used as markers and never match. */ @@ -48,7 +48,7 @@ libc_hidden_def (__file_is_unchanged) *ST. */ void __file_change_detection_for_stat (struct file_change_detection *file, - const struct stat64 *st) + const struct stat64 *st) { if (S_ISDIR (st->st_mode)) /* Treat as empty file. */ @@ -72,7 +72,7 @@ libc_hidden_def (__file_change_detection_for_stat) false is returned. */ bool __file_change_detection_for_path (struct file_change_detection *file, - const char *path) + const char *path) { struct stat64 st; if (stat64 (path, &st) != 0) @@ -84,13 +84,13 @@ __file_change_detection_for_path (struct file_change_detection *file, case ENOENT: case ENOTDIR: case EPERM: - /* Ignore errors due to file system contents. Instead, treat - the file as empty. */ - file->size = 0; - return true; + /* Ignore errors due to file system contents. Instead, treat + the file as empty. */ + file->size = 0; + return true; default: - /* Other errors are fatal. */ - return false; + /* Other errors are fatal. */ + return false; } else /* stat64 was successfull. */ { @@ -105,7 +105,7 @@ libc_hidden_def (__file_change_detection_for_path) as non-existing. */ bool __file_change_detection_for_fp (struct file_change_detection *file, - FILE *fp) + FILE *fp) { if (fp == NULL) { @@ -117,13 +117,13 @@ __file_change_detection_for_fp (struct file_change_detection *file, { struct stat64 st; if (fstat64 (__fileno (fp), &st) != 0) - /* If we already have a file descriptor, all errors are fatal. */ - return false; + /* If we already have a file descriptor, all errors are fatal. */ + return false; else - { - __file_change_detection_for_stat (file, &st); - return true; - } + { + __file_change_detection_for_stat (file, &st); + return true; + } } } libc_hidden_def (__file_change_detection_for_fp)