Add new align_func_params_star_gap option

[halfline]

This commit adds a new option, align_func_params_star_gap, that when paired with align_var_dev_star_style = dangle, provides a way to specify a minimum gap that stars in function parameters won't dangle into.

Closes: #3770

[halfline]

I don't claim to really grok the code, this pull request is a product of "guess and check" :-) so please review carefully!

[micheleCTDEAdmin]

Can you make sure etc/uigui_uncrustify.ini is build in release mode? The only changes there should be the new option, without affecting all the others. You can see instead that numbers have been added to each option, which happens in debug mode.

[micheleCTDEAdmin]

I converted this to DRAFT till we further understand the bug described here. We shall first address that bug, then revisit the need for this additional option.

[halfline]

Okay, I wasn't entirely sure how to accomplish what you were asking, but I think I figured it out:

╎❯ mkdir release
╎❯ (cd release; cmake -DCMAKE_BUILD_TYPE=Release ..; make)
╎❯ python3 ./scripts/release_tool.py optiondocs release/uncrustify

[halfline]

(note we should rebase and rework this on top of #3893 before merging)

[halfline]

okay, I won't say I'm 100% confident this is right, but it passes the tests, and I think it's in better shape than before. Dropping draft status following discussion from #3770

[micheleCTDEAdmin]

@halfline
the PR in itself is good and many thanks for updating the comments on several options as we discussed in #3770.
There is one thing that I think it is worth discussing: whether to have two parameters for the gap or to combine them together into one. Two parameters give more flexibility, but maybe users may get confused or wonder why there are two parameters or how the two gaps overlap at times. Also how about &? Are we going to have an option for the gap for ampersand too? and how does that combine with the star option if we have both stars and &s in the same alignment stack but the options have different values?
I was thinking it may make more sense to have the additional gap provided by align_func_params_star_gap as part of the gap provided by align_func_params_gap: if the alignment style option for star and/or ampersand is 2, then the align_func_params_star_gap would be the gap between the type and the left-most star or ampersand.
What do you think about it?

[halfline]

So first let me make sure I'm following you. You wrote:

I was thinking it may make more sense to have the additional gap provided by align_func_params_star_gap as part of the gap provided by align_func_params_gap: if the alignment style option for star and/or ampersand is 2, then the align_func_params_star_gap would be the gap between the type and the left-most star or ampersand.
What do you think about it?

but if I'm reading you right, I think you actually meant:

I was thinking it may make more sense to have the additional gap provided by align_func_params_star_gap as part of the gap provided by align_func_params_gap: if the alignment style option for star and/or ampersand is 2, then the align_func_params_gap would be the gap between the type and the left-most star or ampersand.
What do you think about it?

right? My big issue with that is it's a pretty big semantic change. SS_DANGLE would no longer mean dangle stars in the gap defined by align_func_params_gap, but instead mean change the meaning of align_func_params_gap to something else entirely. SS_DANGLE would no longer be a good name, so for clarity, we would probably need to rename it, but I struggle to think of a name that would fit the new behavior (SS_GAP_SHIFTER ?). I also fear it may break existing configs in the wild.

Take for instance, someone who wants their function spaced like this:

int   function  (type    input,
                 int   **output)
{
}


int  other  (type    input,
             int     output)
{
}

Right now they'd set a gap of 4 and a star style of dangle. If we make your proposed change, it's going to start looking like this:

int   function  (type     input,
                 int    **output)
{
}


int  other  (type    input,
             int     output)
{
}

Notice, there's more space in the top function now. All of sudden existing code bases will start formatting differently. Furthermore, there will be no way to get the old behavior back for people who really wanted the old behavior.

Having said that, having thought about it more, for my use case, I do think what you propose would work okay. So I suppose we could ditch align_func_params_star_gap and add a star style 3 (though we'll have to figure out a name for it) that repurposes the align_func_params_gap to be what I want align_func_params_star_gap to be.

As you said, it does lose flexibility. It means people won't be able to do this from my test case:

int overlapping_gaps(int              one,
                     int             *two,
                     unsigned        *three,
                     long long      **four)
//                  star gap ↗ |1234| ↖ star gap
//           alignment gap ↗ |12345678| ↖ alignment gap
{
       return 0;
}

(keep the name type and value a minimum number of spaces from each other, while also keeping the type name and stars a different minimum number of spaces from each other)

I don't want that in my codebase, so it's not critical for me.

I still have a preference for two different options. It seems to provide the most flexibility and the least chance to break existing code bases.

amperstand is an interesting question...I don't use C++ so it's not something I put much thought into. I suspect it may be be somewhat broken already since AlignStack::Flush doesn't seem to check for m_amp_style == SS_DANGLE in the same way it checks for m_star_style == SS_DANGLE.

I guess we could add some more test cases to see how it behaves

[micheleCTDE]

but if I'm reading you right, I think you actually meant:

Yes, thanks for "reading between the wrong lines" and understand what I meant.

You made some very valid points. After thinking quite a bit more about all the discussion, it may be just easier to leave the PR as you created: the users will decide whether to use the new option or not.
Just a couple of questions before we merge this:

1. how does the align_func_params_star_gap behaves if the alignment style is 0 ir 1?
2. would be good to make the option consider ampersands as well. Many options exists to provide the same functionality for stars and ampersands, so maybe this option could handle both (with a slight name change).

[halfline]

(just a quick note to say this is still on my radar to come back to soonish, but it's pushed down the stack a little, so it might be a few days)

[micheleCTDE]

(just a quick note to say this is still on my radar to come back to soonish, but it's pushed down the stack a little, so it might be a few days)

No worries, ping me here when you are ready with that