{"thread":{"id":"54278","subject":"[PATCH] reftable: fix some sparse warnings","startedAt":"2020-09-22T22:55:06Z","lastAt":"2020-09-30T20:18:20Z","messageCount":3,"participants":["Ramsay Jones","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406162","messageId":"eaf4629b-d8c4-0ddc-8c85-6600399a8229@ramsayjones.plus.com","threadId":"54278","inReplyTo":null,"subject":"[PATCH] reftable: fix some sparse warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-09-22T22:47:31Z","receivedAt":"2020-09-22T22:55:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n\nHi Han-Wen Nienhuys,\n\nIf you need to re-roll your 'hn/reftable' branch, could you please squash this\ninto the relevant patches.\n\nThis patch is based on top of 'seen' and removes 20 sparse warnings (19 of the\n\"symbol 'whatever' was not declared. Should it be static?\" type and a single\n\"Using plain integer as NULL pointer\").\n\nHowever, this branch also adds many symbols to the output of my 'static-check.pl'\nscript (this patch actually removes 19 of those symbols :) ).\n\nLooking at the first such symbol:\n\n  $ git grep -n block_writer_register_restart\n  reftable/block.c:40:int block_writer_register_restart(struct block_writer *w, int n, int restart,\n  reftable/block.c:93:    if (block_writer_register_restart(w, start.len - out.len, is_restart,\n  reftable/block.c:105:int block_writer_register_restart(struct block_writer *w, int n, int is_restart,\n  $ \n\n... it looks like you forward declare the function on line 40, use it in line 93\nand define it in line 105, all in the single 'block.c' source file. This would\nsuggest that this symbol is local to this file and should be marked as 'static'.\n(Also, unless you have mutually recursive functions, I would avoid the need to\nforward declare this function).\n\nJust for your information, you may want to look at the following 27 symbols:\n\n  $ diff nsc ssc1\n  ...\n  > reftable/block.o\t- block_writer_register_restart\n  > reftable/merged.o\t- merged_table_seek_record\n  > reftable/merged.o\t- reftable_merged_table_hash_id\n  > reftable/merged.o\t- reftable_merged_table_max_update_index\n  > reftable/merged.o\t- reftable_merged_table_min_update_index\n  > reftable/merged.o\t- reftable_merged_table_seek_log_at\n  > reftable/pq.o\t- pq_less\n  > reftable/publicbasics.o\t- reftable_error_to_errno\n  > reftable/publicbasics.o\t- reftable_set_alloc\n  > reftable/reader.o\t- reftable_reader_seek_log_at\n  > reftable/record.o\t- reftable_record_yield\n  > reftable/record.o\t- reftable_ref_record_vtable\n  > reftable/refname.o\t- modification_has_ref\n  > reftable/refname.o\t- modification_has_ref_with_prefix\n  > reftable/refname.o\t- validate_refname\n  > reftable/stack.o\t- reftable_addition_close\n  > reftable/stack.o\t- reftable_stack_auto_compact\n  > reftable/stack.o\t- reftable_stack_reload_maybe_reuse\n  > reftable/stack.o\t- stack_check_addition\n  > reftable/stack.o\t- stack_try_add\n  > reftable/stack.o\t- stack_write_compact\n  > reftable/test_framework.o\t- new_test_case\n  > reftable/writer.o\t- reftable_writer_add_logs\n  > reftable/writer.o\t- reftable_writer_add_refs\n  > reftable/writer.o\t- writer_clear_index\n  > reftable/writer.o\t- writer_finish_public_section\n  > reftable/writer.o\t- writer_flush_block\n  ...\n  $ \n\nTo be clear, all of the above symbols are defined, as an external symbol, in\nthe given object file, but not called/referenced outside of that file.\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n reftable/blocksource.c    | 8 ++++----\n reftable/iter.c           | 6 +++---\n reftable/merged.c         | 2 +-\n reftable/publicbasics.c   | 6 +++---\n reftable/reader.c         | 2 +-\n reftable/record.c         | 6 +++---\n reftable/test_framework.c | 8 ++++----\n reftable/writer.c         | 2 +-\n 8 files changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex f12cea2472..7f29b864f9 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -39,7 +39,7 @@ static uint64_t strbuf_size(void *b)\n \treturn ((struct strbuf *)b)->len;\n }\n \n-struct reftable_block_source_vtable strbuf_vtable = {\n+static struct reftable_block_source_vtable strbuf_vtable = {\n \t.size = &strbuf_size,\n \t.read_block = &strbuf_read_block,\n \t.return_block = &strbuf_return_block,\n@@ -60,11 +60,11 @@ static void malloc_return_block(void *b, struct reftable_block *dest)\n \treftable_free(dest->data);\n }\n \n-struct reftable_block_source_vtable malloc_vtable = {\n+static struct reftable_block_source_vtable malloc_vtable = {\n \t.return_block = &malloc_return_block,\n };\n \n-struct reftable_block_source malloc_block_source_instance = {\n+static struct reftable_block_source malloc_block_source_instance = {\n \t.ops = &malloc_vtable,\n };\n \n@@ -112,7 +112,7 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n \treturn size;\n }\n \n-struct reftable_block_source_vtable file_vtable = {\n+static struct reftable_block_source_vtable file_vtable = {\n \t.size = &file_size,\n \t.read_block = &file_read_block,\n \t.return_block = &file_return_block,\ndiff --git a/reftable/iter.c b/reftable/iter.c\nindex 6631408ef8..2cff447323 100644\n--- a/reftable/iter.c\n+++ b/reftable/iter.c\n@@ -29,7 +29,7 @@ static void empty_iterator_close(void *arg)\n {\n }\n \n-struct reftable_iterator_vtable empty_vtable = {\n+static struct reftable_iterator_vtable empty_vtable = {\n \t.next = &empty_iterator_next,\n \t.close = &empty_iterator_close,\n };\n@@ -126,7 +126,7 @@ static int filtering_ref_iterator_next(void *iter_arg,\n \treturn err;\n }\n \n-struct reftable_iterator_vtable filtering_ref_iterator_vtable = {\n+static struct reftable_iterator_vtable filtering_ref_iterator_vtable = {\n \t.next = &filtering_ref_iterator_next,\n \t.close = &filtering_ref_iterator_close,\n };\n@@ -228,7 +228,7 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest,\n \treturn err;\n }\n \n-struct reftable_iterator_vtable indexed_table_ref_iter_vtable = {\n+static struct reftable_iterator_vtable indexed_table_ref_iter_vtable = {\n \t.next = &indexed_table_ref_iter_next,\n \t.close = &indexed_table_ref_iter_close,\n };\ndiff --git a/reftable/merged.c b/reftable/merged.c\nindex 22b071cb5d..63fa69bc27 100644\n--- a/reftable/merged.c\n+++ b/reftable/merged.c\n@@ -155,7 +155,7 @@ static int merged_iter_next_void(void *p, struct reftable_record *rec)\n \treturn merged_iter_next(mi, rec);\n }\n \n-struct reftable_iterator_vtable merged_iter_vtable = {\n+static struct reftable_iterator_vtable merged_iter_vtable = {\n \t.next = &merged_iter_next_void,\n \t.close = &merged_iter_close,\n };\ndiff --git a/reftable/publicbasics.c b/reftable/publicbasics.c\nindex a31463ff9a..12d547d70d 100644\n--- a/reftable/publicbasics.c\n+++ b/reftable/publicbasics.c\n@@ -59,9 +59,9 @@ int reftable_error_to_errno(int err)\n \t}\n }\n \n-void *(*reftable_malloc_ptr)(size_t sz) = &malloc;\n-void *(*reftable_realloc_ptr)(void *, size_t) = &realloc;\n-void (*reftable_free_ptr)(void *) = &free;\n+static void *(*reftable_malloc_ptr)(size_t sz) = &malloc;\n+static void *(*reftable_realloc_ptr)(void *, size_t) = &realloc;\n+static void (*reftable_free_ptr)(void *) = &free;\n \n void *reftable_malloc(size_t sz)\n {\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex fae2dbb64e..c7f56b5fdc 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -376,7 +376,7 @@ static void table_iter_close(void *p)\n \tblock_iter_close(&ti->bi);\n }\n \n-struct reftable_iterator_vtable table_iter_vtable = {\n+static struct reftable_iterator_vtable table_iter_vtable = {\n \t.next = &table_iter_next_void,\n \t.close = &table_iter_close,\n };\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 21c9bba077..3b6884131b 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -539,7 +539,7 @@ static int not_a_deletion(const void *p)\n \treturn 0;\n }\n \n-struct reftable_record_vtable reftable_obj_record_vtable = {\n+static struct reftable_record_vtable reftable_obj_record_vtable = {\n \t.key = &reftable_obj_record_key,\n \t.type = BLOCK_TYPE_OBJ,\n \t.copy_from = &reftable_obj_record_copy_from,\n@@ -821,7 +821,7 @@ static int reftable_log_record_is_deletion_void(const void *p)\n \t\t(const struct reftable_log_record *)p);\n }\n \n-struct reftable_record_vtable reftable_log_record_vtable = {\n+static struct reftable_record_vtable reftable_log_record_vtable = {\n \t.key = &reftable_log_record_key,\n \t.type = BLOCK_TYPE_LOG,\n \t.copy_from = &reftable_log_record_copy_from,\n@@ -947,7 +947,7 @@ static int reftable_index_record_decode(void *rec, struct strbuf key,\n \treturn start.len - in.len;\n }\n \n-struct reftable_record_vtable reftable_index_record_vtable = {\n+static struct reftable_record_vtable reftable_index_record_vtable = {\n \t.key = &reftable_index_record_key,\n \t.type = BLOCK_TYPE_INDEX,\n \t.copy_from = &reftable_index_record_copy_from,\ndiff --git a/reftable/test_framework.c b/reftable/test_framework.c\nindex f304a2773a..b5870bea08 100644\n--- a/reftable/test_framework.c\n+++ b/reftable/test_framework.c\n@@ -11,9 +11,9 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"basics.h\"\n \n-struct test_case **test_cases;\n-int test_case_len;\n-int test_case_cap;\n+static struct test_case **test_cases;\n+static int test_case_len;\n+static int test_case_cap;\n \n struct test_case *new_test_case(const char *name, void (*testfunc)(void))\n {\n@@ -56,7 +56,7 @@ int test_main(int argc, const char *argv[])\n \t\treftable_free(test_cases[i]);\n \t}\n \treftable_free(test_cases);\n-\ttest_cases = 0;\n+\ttest_cases = NULL;\n \ttest_case_len = 0;\n \ttest_case_cap = 0;\n \treturn 0;\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 44ddcc6757..f569d15ff0 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -589,7 +589,7 @@ void writer_clear_index(struct reftable_writer *w)\n \tw->index_cap = 0;\n }\n \n-const int debug = 0;\n+static const int debug;\n \n static int writer_flush_nonempty_block(struct reftable_writer *w)\n {\n-- \n2.28.0\n"},{"id":"406695","messageId":"CAFQ2z_PNT0g2yAbMkWEQW3UjbMbzALfkAfLDzpiUZjOE0b29vw@mail.gmail.com","threadId":"54278","inReplyTo":"eaf4629b-d8c4-0ddc-8c85-6600399a8229@ramsayjones.plus.com","subject":"Re: [PATCH] reftable: fix some sparse warnings","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-09-30T16:51:24Z","receivedAt":"2020-09-30T16:51:39Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Sep 23, 2020 at 12:47 AM Ramsay Jones\n<ramsay@ramsayjones.plus.com> wrote:\n>\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>\n> Hi Han-Wen Nienhuys,\n>\n> If you need to re-roll your 'hn/reftable' branch, could you please squash this\n> into the relevant patches.\n>\n\nThanks for the heads-up. I fixed some of these issues in the source at\ngoogle/reftable. I've seen a Helped-By footer used to acknowledge\nthese types of contributions, but I'm not sure on which of the 13\ncommits I should put that; suggestions?\n\n> This patch is based on top of 'seen' and removes 20 sparse warnings (19 of the\n\nCould you tell me how I can run these checks myself?\n\n> Just for your information, you may want to look at the following 27 symbols:\n\n>   > reftable/merged.o   - reftable_merged_table_hash_id\n>   > reftable/merged.o   - reftable_merged_table_max_update_index\n>   > reftable/merged.o   - reftable_merged_table_min_update_index\n>   > reftable/merged.o   - reftable_merged_table_seek_log_at\n>   > reftable/publicbasics.o     - reftable_error_to_errno\n>   > reftable/publicbasics.o     - reftable_set_alloc\n>   > reftable/reader.o   - reftable_reader_seek_log_at\n>  > reftable/stack.o    - reftable_addition_close\n>   > reftable/stack.o    - reftable_stack_auto_compact\n\nThese functions are part of the public API. We'll need to get the\nreftable glue code into seen. Perhaps some need unittest coverage too.\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"406704","messageId":"d3c6bd3d-9f1c-00e3-fa31-67ad0d45fdbf@ramsayjones.plus.com","threadId":"54278","inReplyTo":"CAFQ2z_PNT0g2yAbMkWEQW3UjbMbzALfkAfLDzpiUZjOE0b29vw@mail.gmail.com","subject":"Re: [PATCH] reftable: fix some sparse warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-09-30T20:18:14Z","receivedAt":"2020-09-30T20:18:20Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/09/2020 17:51, Han-Wen Nienhuys wrote:\n> On Wed, Sep 23, 2020 at 12:47 AM Ramsay Jones\n> <ramsay@ramsayjones.plus.com> wrote:\n\n>> If you need to re-roll your 'hn/reftable' branch, could you please squash this\n>> into the relevant patches.\n>>\n> \n> Thanks for the heads-up. I fixed some of these issues in the source at\n> google/reftable. I've seen a Helped-By footer used to acknowledge\n> these types of contributions, but I'm not sure on which of the 13\n> commits I should put that; suggestions?\n\nI will leave you to decide, but I didn't actually do much (sparse\ndid most of the heavy lifting)! ;-)\n\n> \n>> This patch is based on top of 'seen' and removes 20 sparse warnings (19 of the\n> \n> Could you tell me how I can run these checks myself?\n\n  $ make sparse\n\nYou will need to install a suitably new version of sparse, of course.\nI believe (but don't quote me) that debian testing has a suitable\nversion based on the 'maint-v0.6.2' branch. ('git describe maint-v0.6.2'\nshows that it has four commits on top of the last official release\nversion: v0.6.2-4-gb47eba20). Having said that, v0.6.2 should also be\nfine (I build from source, so have version v0.6.2-201-g24bdaac6).\n\n[for more sparse info, see: https://sparse.docs.kernel.org]\n\n> \n>> Just for your information, you may want to look at the following 27 symbols:\n> \n>>   > reftable/merged.o   - reftable_merged_table_hash_id\n>>   > reftable/merged.o   - reftable_merged_table_max_update_index\n>>   > reftable/merged.o   - reftable_merged_table_min_update_index\n>>   > reftable/merged.o   - reftable_merged_table_seek_log_at\n>>   > reftable/publicbasics.o     - reftable_error_to_errno\n>>   > reftable/publicbasics.o     - reftable_set_alloc\n>>   > reftable/reader.o   - reftable_reader_seek_log_at\n>>  > reftable/stack.o    - reftable_addition_close\n>>   > reftable/stack.o    - reftable_stack_auto_compact\n> \n> These functions are part of the public API. We'll need to get the\n> reftable glue code into seen. Perhaps some need unittest coverage too.\n\nSo, do I take it that the other 18 symbols are now marked 'static'?\n\n[you would need 'static-check.pl' to catch symbols like the above].\n\nATB,\nRamsay Jones\n\n\n"}]}