git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/9] commit-graph: stop using signed integers to count bloom filters

From
Patrick Steinhardt <ps@pks.im>
Date
Aug 7, 2025, 07:04 UTC
Message-ID
<aJRP_muuzCAy3mzF@pks.im>
In-Reply-To
<xmqq4iukwj83.fsf@gitster.g>
On Wed, Aug 06, 2025 at 08:41:32AM -0700, Junio C Hamano wrote:
Show 37 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> I wrote these counters in 312cff5207 (bloom: split 'get_bloom_filter()'
> >> in two, 2020-09-16) and 59f0d5073f (bloom: encode out-of-bounds filters
> >> as non-empty, 2020-09-17), and I don't see a compelling reason that
> >> these should be unsigned.
> >
> > I think that is going backwards though: the question to ask is why
> > should these be signed if they cannot ever be negative?
> 
> Earlier I gave an example of allowing for a "not yet counted"
> sentinel value for a variable or a structure member.  Another
> example may be for a function that counts that also needs to signal
> an error, and as usual in any C programs, the natural way to do so
> for any function whose "normal" return values are non-negative
> integers is to signal errors with a negative value.
> 
> Note that a structure member or a variable that does not need such a
> "not yet counted" sentinel value (e.g., it may have a separate
> "counted already" member associated with it, or the nature of the
> thing it counts does not have such "not yet counted" state), and it
> is possible for such a variable to live happily with a function that
> can signal an error.
> 
> It means the variable that receives the counted result from such a
> function may be able to use only half a range of values as its type
> implies, if that helper function is the only source of information
> that is assigned to it, though.
> 
> If the counter in question never needs to store such a sentinel
> value itself, then I am OK for it to be unsigned, and that is
> exactly why I said "not always a valid excuse".  But if the counter
> variable or structure member has to work with functions that need to
> return sentinel values (like platform natural int that can use the
> usual "negative is an error, non-negative is a normal result"), it
> may have less chance to trigger the -Wsign-compare irritation, if
> you made it also signed.

Yup, fully agreed, and this is a good reason why it should be signed. In the case at hand though we never use such sentinel values, and I think making that explicit by using an unsigned type is a good thing as it tells the reader that "Yup, no sentinels involved, it's a plain counter from 0 to $NUM_ENTRIES".

Patrick
Previous: Junio C HamanoNext: Junio C Hamano
Message 13 of 69 in “commit-graph: remove reliance on global state”
  1. 0/9 commit-graph: remove reliance on global statePatrick Steinhardt, Aug 4, 2025
  2. 1/9 trace2: introduce function to trace unsigned integersPatrick Steinhardt, Aug 4, 2025
  3. Taylor BlauAug 4, 2025
  4. 2/9 commit-graph: stop using signed integers to count bloom filtersPatrick Steinhardt, Aug 4, 2025
  5. Oswald BuddenhagenAug 4, 2025
  6. Patrick SteinhardtAug 4, 2025
  7. Junio C HamanoAug 4, 2025
  8. Taylor BlauAug 4, 2025
  9. Patrick SteinhardtAug 6, 2025
  10. Oswald BuddenhagenAug 6, 2025
  11. Junio C HamanoAug 6, 2025
  12. Junio C HamanoAug 6, 2025
  13. Patrick SteinhardtAug 7, 2025
  14. Junio C HamanoAug 7, 2025
  15. Patrick SteinhardtAug 11, 2025
  16. Junio C HamanoAug 5, 2025
  17. Taylor BlauAug 4, 2025
  18. 3/9 commit-graph: fix type for some write optionsPatrick Steinhardt, Aug 4, 2025
  19. Taylor BlauAug 4, 2025
  20. 4/9 commit-graph: fix sign comparison warningsPatrick Steinhardt, Aug 4, 2025
  21. Taylor BlauAug 4, 2025
  22. Patrick SteinhardtAug 6, 2025
  23. 5/9 commit-graph: stop using `the_hash_algo` via macrosPatrick Steinhardt, Aug 4, 2025
  24. Taylor BlauAug 4, 2025
  25. 6/9 commit-graph: store the hash algorithm instead of its lengthPatrick Steinhardt, Aug 4, 2025
  26. Taylor BlauAug 4, 2025
  27. 7/9 commit-graph: stop using `the_hash_algo`Patrick Steinhardt, Aug 4, 2025
  28. Taylor BlauAug 4, 2025
  29. Patrick SteinhardtAug 6, 2025
  30. 8/9 commit-graph: stop using `the_repository`Patrick Steinhardt, Aug 4, 2025
  31. Taylor BlauAug 4, 2025
  32. 9/9 commit-graph: stop passing in redundant repositoryPatrick Steinhardt, Aug 4, 2025
  33. Derrick StoleeAug 5, 2025
  34. Patrick SteinhardtAug 6, 2025
  35. 00/10 commit-graph: remove reliance on global statePatrick Steinhardt, Aug 6, 2025
  36. 01/10 trace2: introduce function to trace unsigned integersPatrick Steinhardt, Aug 6, 2025
  37. 02/10 commit-graph: stop using signed integers to count Bloom filtersPatrick Steinhardt, Aug 6, 2025
  38. 03/10 commit-graph: fix type for some write optionsPatrick Steinhardt, Aug 6, 2025
  39. Oswald BuddenhagenAug 6, 2025
  40. Junio C HamanoAug 6, 2025
  41. Patrick SteinhardtAug 7, 2025
  42. 04/10 commit-graph: fix sign comparison warningsPatrick Steinhardt, Aug 6, 2025
  43. 05/10 commit-graph: stop using `the_hash_algo` via macrosPatrick Steinhardt, Aug 6, 2025
  44. 06/10 commit-graph: store the hash algorithm instead of its lengthPatrick Steinhardt, Aug 6, 2025
  45. 07/10 commit-graph: refactor `parse_commit_graph()` to take a repositoryPatrick Steinhardt, Aug 6, 2025
  46. 08/10 commit-graph: stop using `the_hash_algo`Patrick Steinhardt, Aug 6, 2025
  47. 09/10 commit-graph: stop using `the_repository`Patrick Steinhardt, Aug 6, 2025
  48. 10/10 commit-graph: stop passing in redundant repositoryPatrick Steinhardt, Aug 6, 2025
  49. 00/10 commit-graph: remove reliance on global statePatrick Steinhardt, Aug 7, 2025
  50. 01/10 trace2: introduce function to trace unsigned integersPatrick Steinhardt, Aug 7, 2025
  51. 02/10 commit-graph: stop using signed integers to count Bloom filtersPatrick Steinhardt, Aug 7, 2025
  52. 03/10 commit-graph: fix type for some write optionsPatrick Steinhardt, Aug 7, 2025
  53. Junio C HamanoAug 7, 2025
  54. Patrick SteinhardtAug 11, 2025
  55. 04/10 commit-graph: fix sign comparison warningsPatrick Steinhardt, Aug 7, 2025
  56. 05/10 commit-graph: stop using `the_hash_algo` via macrosPatrick Steinhardt, Aug 7, 2025
  57. 06/10 commit-graph: store the hash algorithm instead of its lengthPatrick Steinhardt, Aug 7, 2025
  58. 07/10 commit-graph: refactor `parse_commit_graph()` to take a repositoryPatrick Steinhardt, Aug 7, 2025
  59. 08/10 commit-graph: stop using `the_hash_algo`Patrick Steinhardt, Aug 7, 2025
  60. 09/10 commit-graph: stop using `the_repository`Patrick Steinhardt, Aug 7, 2025
  61. 10/10 commit-graph: stop passing in redundant repositoryPatrick Steinhardt, Aug 7, 2025
  62. 0/6 commit-graph: remove reliance on global statePatrick Steinhardt, Aug 15, 2025
  63. 1/6 commit-graph: stop using `the_hash_algo` via macrosPatrick Steinhardt, Aug 15, 2025
  64. 2/6 commit-graph: store the hash algorithm instead of its lengthPatrick Steinhardt, Aug 15, 2025
  65. 3/6 commit-graph: refactor `parse_commit_graph()` to take a repositoryPatrick Steinhardt, Aug 15, 2025
  66. 4/6 commit-graph: stop using `the_hash_algo`Patrick Steinhardt, Aug 15, 2025
  67. 5/6 commit-graph: stop using `the_repository`Patrick Steinhardt, Aug 15, 2025
  68. 6/6 commit-graph: stop passing in redundant repositoryPatrick Steinhardt, Aug 15, 2025
  69. Derrick StoleeAug 15, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.