| Message ID | 20200423205851.GA1633985@coredump.intra.peff.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | minor fixups for gs/commit-graph-path-filter | expand |
On 4/23/2020 4:58 PM, Jeff King wrote: > These are just a few bits I noticed in the test-tool helper when the > topic hit next (my -Wunused-parameter patch complained that we never > looked at argc). > > [1/2]: test-bloom: fix some whitespace issues > [2/2]: test-bloom: check that we have expected arguments > > t/helper/test-bloom.c | 28 ++++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > -Peff > Thank you for doing this! Both patches look good to me. I also don't care about the brace/no-brace thing that Taylor brought up for 1/2. Cheers! Garima Singh
On 2020-04-23 16:58:51-0400, Jeff King <peff@peff.net> wrote: > These are just a few bits I noticed in the test-tool helper when the > topic hit next (my -Wunused-parameter patch complained that we never > looked at argc). I think I'll add this one to those few bits. Old version of this change was sent here: <20200423133937.GA1984@danh.dev> But that version doesn't have the fixup for sh script. Garima Singh: Could you please change your editor to add final new line? I've take another look into bloom.h. I think we should drop BITS_PER_WORD definition and use CHAR_BIT instead. It's a standard definition. To me, a WORD is an `int`, at least I was told that when I was still in university and study about computer science. -----------------8<--------------------- From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= <congdanhqx@gmail.com> Date: Thu, 23 Apr 2020 20:24:50 +0700 Subject: [PATCH] bloom: fix `make sparse` warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * We need a `final_new_line` to make our source code as text file, per POSIX and C specification. * `bloom_filters` should be limited to interal linkage only Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- Feel free to fix up to current series bloom.c | 4 ++-- bloom.h | 2 +- t/helper/test-bloom.c | 2 +- t/t0095-bloom.sh | 2 +- t/t4216-log-bloom.sh | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/bloom.c b/bloom.c index dd9bab9bbd..ee025e0c61 100644 --- a/bloom.c +++ b/bloom.c @@ -9,7 +9,7 @@ define_commit_slab(bloom_filter_slab, struct bloom_filter); -struct bloom_filter_slab bloom_filters; +static struct bloom_filter_slab bloom_filters; struct pathmap_hash_entry { struct hashmap_entry entry; @@ -273,4 +273,4 @@ int bloom_filter_contains(const struct bloom_filter *filter, } return 1; -} \ No newline at end of file +} diff --git a/bloom.h b/bloom.h index b935186425..e0e59e0754 100644 --- a/bloom.h +++ b/bloom.h @@ -87,4 +87,4 @@ int bloom_filter_contains(const struct bloom_filter *filter, const struct bloom_key *key, const struct bloom_filter_settings *settings); -#endif \ No newline at end of file +#endif diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c index 77eb27adac..456f5ea7f9 100644 --- a/t/helper/test-bloom.c +++ b/t/helper/test-bloom.c @@ -3,7 +3,7 @@ #include "test-tool.h" #include "commit.h" -struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS; +static struct bloom_filter_settings settings = DEFAULT_BLOOM_FILTER_SETTINGS; static void add_string_to_filter(const char *data, struct bloom_filter *filter) { struct bloom_key key; diff --git a/t/t0095-bloom.sh b/t/t0095-bloom.sh index 8f9eef116d..809ec7b0b8 100755 --- a/t/t0095-bloom.sh +++ b/t/t0095-bloom.sh @@ -114,4 +114,4 @@ test_expect_success EXPENSIVE 'get bloom filter for commit with 513 changes' ' test_cmp expect actual ' -test_done \ No newline at end of file +test_done diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index c7011f33e2..21b68dd6c8 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -152,4 +152,4 @@ test_expect_success 'Use Bloom filters if they exist in the latest but not all c test_bloom_filters_used_when_some_filters_are_missing "-- A/B" ' -test_done \ No newline at end of file +test_done
On Fri, Apr 24, 2020 at 08:00:47AM +0700, Danh Doan wrote: > On 2020-04-23 16:58:51-0400, Jeff King <peff@peff.net> wrote: > > These are just a few bits I noticed in the test-tool helper when the > > topic hit next (my -Wunused-parameter patch complained that we never > > looked at argc). > > I think I'll add this one to those few bits. Yeah, they all look sensible (I should have looked for more "No newline" cases. > I've take another look into bloom.h. > > I think we should drop BITS_PER_WORD definition and use CHAR_BIT > instead. It's a standard definition. > > To me, a WORD is an `int`, at least I was told that when I was still > in university and study about computer science. Yes, I agree it would be more clear as just CHAR_BIT if we are using single-char words. But I suspect the code could be looking at the bit patterns using larger word sizes (e.g., all of the ewah code uses 64-bit words). That might be worth exploring. -Peff
On Thu, Apr 23, 2020 at 06:14:36PM -0400, Garima Singh wrote: > > On 4/23/2020 4:58 PM, Jeff King wrote: > > These are just a few bits I noticed in the test-tool helper when the > > topic hit next (my -Wunused-parameter patch complained that we never > > looked at argc). > > > > [1/2]: test-bloom: fix some whitespace issues > > [2/2]: test-bloom: check that we have expected arguments > > > > t/helper/test-bloom.c | 28 ++++++++++++++++++++-------- > > 1 file changed, 20 insertions(+), 8 deletions(-) > > > > -Peff > > > > Thank you for doing this! > Both patches look good to me. > I also don't care about the brace/no-brace thing that > Taylor brought up for 1/2. To be clear, I don't care about them either ;). Maybe it's time that we relax that rule (if it seems that a good number of us don't mind it either way)..? > Cheers! > Garima Singh Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > To be clear, I don't care about them either ;). Maybe it's time that we > relax that rule (if it seems that a good number of us don't mind it > either way)..? I'd prefer our reviewers (even more preferably the authors) to be careful about new code, but it probably is not worth reviewing patches that only fix them and do nothing else.