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

Re: [PATCH 0/2] strbuf: improve API

From
WDWilliam Duclot <william.duclot@ensimag.grenoble-inp.fr>
Date
Jun 2, 2016, 14:22 UTC
Message-ID
<20160602142205.GA5566@Messiaen>
In-Reply-To
<vpqwpm7n3wx.fsf@anie.imag.fr>
On Thu, Jun 02, 2016 at 02:58:22PM +0200, Matthieu Moy wrote:
Show 34 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 1. The amount of added code complexity is small and quite
>>    encapsulated.
> 
> Actually, STRBUF_OWNS_MEMORY can even be seen as a simplification if
> done properly: we already have the case where the strbuf does not own
> the memory with strbuf_slopbuf. I already pointed places in
> strbuf_grow() which could be simplified after the patch. Re-reading the
> code it seems at lesat the call to strbuf_grow(sb, 0); in strbuf_detach
> becomes useless. The same in strbuf_attach() probably is, too.
> 
> So, the final strbuf.[ch] code might not be "worse" that the previous.
> 
> I'm unsure about the complexity of the future code using the new API. I
> don't forsee cases where using the new API would lead to a high
> maintenance cost, but I don't claim I considered all possible uses.
> 
>> 2. The ability to use strbufs without having to allocate memory might
>>    make enough *psychological* difference that it encourages some
>>    devs to use strbufs where they would otherwise have done manual
>>    memory management. I think this would be a *big* win in terms of
>>    potential bugs and security vulnerabilities avoided.
> 
> Note that this can also be seen as a counter-argument, since it
> may psychologically encourage people to micro-optimize code and use
> contributors/reviewers neurons to spend time on "shall this be on-stack
> or malloced?".
> 
> I think we already have a tendency to micro-optimize non-critical code
> too much in Git's codebase, so it's not necessarily a step in the right
> direction.
> 
> In conclusion, I don't have a conclusion, sorry ;-).

Thank you all for your input and your tests, those are very valuable! Me and Simon have to take a decision, as this contribution is part of a school project that comes to an end. We won't have the time to create tests that are representative of the use of strbuf in the Git codebase (partly because we lack knowledge of the codebase) to settle on whether this API improvement is useful or not.

Jeff made very good points, and the tests we ran ourselves seem to agree with yours. That being said, the tests made by Michael, more detailed, suggest that there may be room for an improvement of the strbuf API (even thought that's to confront to the reality of the codebase).

Having little time, we will refactor the patch and send it as a V2: this way, if it appears one day that improving the API with stack-allocated memory is indeed useful, the patch will be ready to merge :)

Previous: Matthieu MoyNext: Jeff King
Message 37 of 38 in “strbuf: improve API”
  1. 0/2 strbuf: improve APIWilliam Duclot, May 30, 2016
  2. 1/2 strbuf: add testsWilliam Duclot, May 30, 2016
  3. Johannes SchindelinMay 30, 2016
  4. Simon RabourgMay 30, 2016
  5. Matthieu MoyMay 30, 2016
  6. Michael HaggertyMay 31, 2016
  7. Simon RabourgMay 31, 2016
  8. 2/2 strbuf: allow to use preallocated memoryWilliam Duclot, May 30, 2016
  9. Johannes SchindelinMay 30, 2016
  10. William DuclotMay 30, 2016
  11. Johannes SchindelinMay 31, 2016
  12. Michael HaggertyMay 31, 2016
  13. Johannes SchindelinMay 31, 2016
  14. Michael HaggertyMay 31, 2016
  15. Matthieu MoyMay 30, 2016
  16. William DuclotMay 30, 2016
  17. Matthieu MoyMay 30, 2016
  18. William DuclotMay 30, 2016
  19. Michael HaggertyMay 31, 2016
  20. William DuclotMay 31, 2016
  21. William DuclotJun 3, 2016
  22. Mike HommeyMay 30, 2016
  23. William DuclotMay 30, 2016
  24. Mike HommeyMay 30, 2016
  25. Junio C HamanoMay 31, 2016
  26. WilliamMay 31, 2016
  27. Matthieu MoyMay 31, 2016
  28. William DuclotMay 31, 2016
  29. Remi Galan AlfonsoMay 30, 2016
  30. Jeff KingJun 1, 2016
  31. David TurnerJun 1, 2016
  32. Jeff KingJun 1, 2016
  33. David TurnerJun 1, 2016
  34. Jeff KingJun 1, 2016
  35. Michael HaggertyJun 2, 2016
  36. Matthieu MoyJun 2, 2016
  37. William DuclotJun 2, 2016
  38. Jeff KingJun 24, 2016

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.