Re: [PATCH 1/9] cache-tree: clean up cache_tree_update()
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Jan 20, 2021, 19:10 UTC
- Message-ID
- <bbed0d73-fa9d-92a0-3bc4-1d44263bb2fb@gmail.com>
- In-Reply-To
- <CABPp-BFiNvPaAffQ0uBfpkHY1WM2gR3ssvpwzxpDhrZmZZjiDA@mail.gmail.com>
On 1/20/2021 12:21 PM, Elijah Newren wrote:
Show 19 quoted lines
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote:... >> + >> + if (!istate->cache_tree) >> + istate->cache_tree = cache_tree(); > > This is the only substantive change. It seems fairly innocuous, but > it makes me wonder the reasoning...I don't know/remember enough about > cache_tree handling to know when this would or wouldn't have already > been allocated. It seems that this would have had to segfault below > if istate->cache_tree were ever NULL, and I don't see you mentioning > any bug you are fixing, so I presume this means you are going to be > adding new codepaths somewhere that cause this function to be reached > under different circumstances than previously had been and you need it > to be more safe for those. Is that correct? Or is it just an > abundance of caution thing that you're adding? If the latter, any > reason you chose to allocate one rather than assume it's a violation > of design invariants and BUG() instead? (Perhaps the commit message > could add a sentence about the rationale for the extra safety?)
It's something I need in the future when I use the cache_tree_update() in more places. I think I call it two times, and either I need to initialize the cache_tree member outside of both, or just make it a feature of the method that it will re-initialize the cache-tree.
Note: the implementation treats an initialized, but empty cache-tree as "invalid" so update_one() correctly populates the full tree.
Thanks, -Stolee