Taylor Blau <me@xxxxxxxxxxxx> writes: > On Mon, Oct 21, 2024 at 02:59:00PM +0200, Patrick Steinhardt wrote: >> On Mon, Oct 21, 2024 at 02:41:45PM +0200, Karthik Nayak wrote: >> > We often name functions with arbitrary suffixes like `_1` as an >> > extension of another existing function. This created confusion and >> >> Micro-nit: s/created/creates/ >> >> [snip] >> > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines >> > index 30fda4142c..b8e2d30567 100644 >> > --- a/Documentation/CodingGuidelines >> > +++ b/Documentation/CodingGuidelines >> > @@ -621,6 +621,11 @@ For C programs: >> > - `S_free()` releases a structure's contents and frees the >> > structure. >> > >> > + - Function names should be self-explanatory, clearly reflecting their >> > + purpose or behavior. To maintain clarity and avoid confusion, >> > + arbitrary suffixes such as _1 are discouraged, as they provide no >> > + meaningful insight into the function's role. >> >> Names being self-explanatory is certainly a worthwhile goal, even though >> I guess that it's a bit more on the idealized side of things. Functions >> will often not be fully self-explanatory, which is likely just a matter >> of reality. I mostly just don't want us to end on the other side of the >> spectrum where we go militant on "Every function must be no longer than >> 2 lines of code such that it can be self-explanatory". >> >> And yes, I'm of course stretching what you are saying quite a bit, I >> know that this is not what you want to say. I'm merely writing down my >> own thoughts while thinking it through. >> >> So, that being said, I agree that we shouldn't use arbitrary suffixes, >> as these are quite hard to understand indeed and typically don't really >> provide any context. So as long as we interpret this rule leniently I'm >> happy with it. > > I am not sure here... I think that using a "_1()" suffix means that the > function is processing one of a number of elements that all need to be > handled similarly, but where both the processing of an individual > element, and the handling of a group of elements are both complicated > enough to be placed in their own function. > The crux here is that this meaning is internalized by people who know the code through in and out. But for anyone new to the project, this is not evident from the naming. > It's also a helpful convention when you have a recursive function that > does some setup during its initial call, but subsequent layers of > recursion don't need or want to repeat that setup. > I get the usecase of having such functions. I totally see the need, it's mostly the naming that I'm trying to change. Let's consider two of such functions: 1. mark_remote_island_1: This function is called to do _some_ work on a single remote island when iterating over a list. 2. find_longest_prefixes_1: This is a recursive function which is used to find the longest prefix. If you notice, both use the "_1" suffix and do different things (operate on a single instance from a list vs provide recursive behavior). So the user has to read the code to understand, which makes the "_1" a bit confusing. Second, this could have instead been named: 1. mark_single_remote_island: Which reads better, giving the idea that we are really working on a single remote island. Whereas having a "_1" doesn't easily imply the same. 2. find_longest_prefixes_recursively: Which also reads better, and gives a hint on how the function operates and why it is split out from the base function. > So I'm not sure I agree that "_1()" is always a bad idea as this changes > suggests (i.e. by writing that "they provide no meaningful insight into > the function's role"). > > Perhaps we could rephrase what is written here to suggest a couple of > instances where we wouldn't want to apply this rule, and the two that I > have written above could perhaps be a useful starting point. But I lean > more towards not adjusting our CodingGuidelines at all here. > I hope my reasoning convinces you of the benefits, this is mostly coming from me spending more time than I'd like to admit on one of these functions, trying to understand what they do :D > Thanks, > Taylor Thanks, Karthik
Attachment:
signature.asc
Description: PGP signature
