Date: Mon, 8 Jan 2018 15:54:45 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327699 - head/sys/sys Message-ID: <89964e3f-a982-f0e5-a7ff-9c13a5ebe61c@FreeBSD.org> In-Reply-To: <CAG6CVpWKWtap3g0rU49NqctKy7Q0d9CR8eS9pNfqxtUXwvvwqQ@mail.gmail.com> References: <201801081609.w08G9941022351@pdx.rh.CN85.dnsmgr.net> <c6cfb6ae-3be7-db5a-bc2e-bf79d558e338@FreeBSD.org> <CAG6CVpWKWtap3g0rU49NqctKy7Q0d9CR8eS9pNfqxtUXwvvwqQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Hello; On 08/01/2018 15:27, Conrad Meyer wrote: > I'm (again, atypically) with rgrimes here. Revert has a specific > meaning and shouldn't be used like this. Making a commit with the > subject "Revert rXXX" that does more than just "svn revert rXXX" makes > life harder for developers looking at code history after you. Making > two separate commits for two different changes (revert, then add the > annotation) is not burdensome. > > Best, > Conrad Yeah, I understand where that comes from and I will take it into account for future commits, but I think it should be *documented* and not assume that everybody thinks that is the way version control is supposed to be used. Pedro. > On Mon, Jan 8, 2018 at 12:18 PM, Pedro Giffuni <pfg@freebsd.org> wrote: >> On 08/01/2018 11:09, Rodney W. Grimes wrote: >>>> Author: pfg >>>> Date: Mon Jan 8 15:54:29 2018 >>>> New Revision: 327699 >>>> URL: https://svnweb.freebsd.org/changeset/base/327699 >>>> >>>> Log: >>>> Revert r327697: >>>> malloc(9): drop the __result_use_check attribute for the kernel >>>> allocator. >>>> My bad: __result_use_check just checks the for the general and we >>>> always >>>> want to make sure allocated memory is used, not only checked for >>>> nullness. >>>> Add it to reallocf since that was missing. >>> Please try not to combine a revert with an add, it makes it messy >>> to try and figure out things in the future when only the svn log >>> is being used to analyze stuff as digging in mail archives becomes >>> painful. >>> >>> Revert, then commit the add standalone, is the better sequence in >>> this type of situation. >> >> Not that any of this is defined in our committers guide but IMHO "svn >> revert" is just a tool, pretty much as "svn move" and "svn copy". The idea >> is to make a committers' life easier: making two commits for such a simple >> change is not "easier". >> >> In this case the change is rather consistent: I added __result_use_check to >> the three functions that needed it. >> The commit log is not only clear on why the revert happened but also >> explains the additional one line change. >> >> When I MFC it, I will merge both changes for repository consistency but the >> log will basically mention that I am adding __result_use_check to >> reallocf(). >> >> >> Pedro. >> >>>> Modified: >>>> head/sys/sys/malloc.h >>>> >>>> Modified: head/sys/sys/malloc.h >>>> >>>> ============================================================================== >>>> --- head/sys/sys/malloc.h Mon Jan 8 15:41:49 2018 (r327698) >>>> +++ head/sys/sys/malloc.h Mon Jan 8 15:54:29 2018 (r327699) >>>> @@ -176,7 +176,7 @@ void *contigmalloc(unsigned long size, struct >>>> malloc_t >>>> __alloc_size(1) __alloc_align(6); >>>> void free(void *addr, struct malloc_type *type); >>>> void *malloc(unsigned long size, struct malloc_type *type, int flags) >>>> - __malloc_like __alloc_size(1); >>>> + __malloc_like __result_use_check __alloc_size(1); >>>> void *mallocarray(size_t nmemb, size_t size, struct malloc_type *type, >>>> int flags) __malloc_like __result_use_check >>>> __alloc_size(1) __alloc_size(2); >>>> @@ -187,9 +187,9 @@ void malloc_type_freed(struct malloc_type >>>> *type, unsig >>>> void malloc_type_list(malloc_type_list_func_t *, void *); >>>> void malloc_uninit(void *); >>>> void *realloc(void *addr, unsigned long size, struct malloc_type >>>> *type, >>>> - int flags) __alloc_size(2); >>>> + int flags) __result_use_check __alloc_size(2); >>>> void *reallocf(void *addr, unsigned long size, struct malloc_type >>>> *type, >>>> - int flags) __alloc_size(2); >>>> + int flags) __result_use_check __alloc_size(2); >>>> struct malloc_type *malloc_desc2type(const char *desc); >>>> #endif /* _KERNEL */ >>>> >>>> >> Pedro. >>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?89964e3f-a982-f0e5-a7ff-9c13a5ebe61c>