> If you are talking about the current specific case of path_value, as > Chet has replied, path_value doesn't return a newly allocated block. > It just returns a pointer to an existing variable cell owned and > managed by the global variable context whose root is static. It > doesn't return or move an ownership of the pointer.
Another reason I wrote it that way because it allowed me to be productive despite my relatively limited knowledge of the codebase. As someone who has just encountered this codebase and is still not fully familiar with it, I thought it was best to err on the side of caution. I've put in effort into making sense of memory managenent abstractions like savestring but it's still a work in progress. > If you are talking about just the coding style in general, it seems > like just your personal style. It is. However, it doesn't seem to be incompatible with the style employed in the bash codebase. Everything is quite well organized and named, it's really good. My intention was only to improve it even further. > the patch just encapsulates it in another function h(x) := f(g(x)) > and the logical structure is the same. Yes, but then you have a nice API which requires less knowledge. It does not require the programmer to know about f and g. This kind of thing could be useful for new contributors who are looking to make improvements despite limited knowledge. Such as myself. Another reason why I added the function: I searched for that function in the course of developing the patch and didn't find it. So I added it. > the choice would depend on many factors > including the maintenance cost > and the preference of the maintainer. Then I would need a _definitive_ decision from Chet Ramey, the maintainer. I am paying attention to his emails. For example, this email factored heavily into my choices: https://lists.gnu.org/archive/html/bug-bash/2024-05/msg00116.html > it should be implemented using a call to find_in_path() > with the appropriate path string and flags. The first thing I did after reading that was look up this find_in_path function which I had somehow missed. I found that indeed it's the right solution and agreed with him. However, I also found that it takes as its string parameter a colon-separated list of directories, not the name of the PATH-like variable that string is bound to. I wanted to dereference a specific variable and then pass the value to this function. I noticed that there was an internal function which does pretty much exactly what I wanted so I simply created an external function for it. I realize that he also said: > I feel like a feature like BASH_SOURCE_PATH > should take less than a dozen lines of code > in source_builtin(). I interpreted that as a preference for shorter code without implying that other solutions (such as mine) are inappropriate. In other words, maybe they would feel differently if the code I submit is good enough. > The final choice of how the code is maintained > would be determined by the maintainer Of course. > If one observes the Bash codebase, one notices that > it is not maintained in your style. That's not my observation at all. I felt very comfortable working with the bash codebase. Everything is organized, well named and neatly separated into logical functions and modules. This is exactly how I like to organize my own projects. I felt so at home it emboldened me to try to improve its general organization. That's why I sent patches to that effect. I didn't think it was unreasonable to create the new find_in_path_var function because there was the very similar find_in_path function just above it. They even accomplish the same things: expose the static functions as a public interface while maintaining the freedom to change the internals. They do essentially the same thing, but the new function takes the name of a bash variable as parameter instead. Nor did I think it was unreasonable to split code up into logical chunks by placing them into helper functions. Those static helpers already exist all over the codebase. Even in builtins/source.def: uw_maybe_pop_dollar_vars. So I don't really understand why there are objections to them. If that's unacceptable, the maintainer just has to say the word. I'll eliminate all this code in the v3 patch set. I just don't see why it would be unacceptable though. -- Matheus
