Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uncrustify doesn't seem to have a way to force gap between pointer parameters and type #3770

Open
halfline opened this issue Jul 29, 2022 · 16 comments · May be fixed by #3889
Open

uncrustify doesn't seem to have a way to force gap between pointer parameters and type #3770

halfline opened this issue Jul 29, 2022 · 16 comments · May be fixed by #3889

Comments

@halfline
Copy link
Contributor

halfline commented Jul 29, 2022

For my projects, I really like function parameters aligned, with stars filling the gap. It seems uncrustify can mostly handle this, except for one complication: I want at least one column of gap space that the stars can't invade. For instance,

static int
this_function_has_a_pointer_type (OtherType  number,
                                  Object    *object)
{
        return 0;
}

You can see there is a two column gap, one to keep the star away from the type, and one column for the star to occupy.
I don't think there's a way to make uncrustify do that right now, without also forcing an extra space for functions that don't have stars like

static int
this_function_has_no_pointer_type (int  number)
{
        return 0;
}

You can see this run:
debug.txt

before:
foo.c

after:
bar.c

╎❯ uncrustify -v
Uncrustify_d-0.75.1_f

I think maybe there could be a new option that will force a column of space between the star and the type, but still allow
the star to dangle in the remainder of the gap?

halfline added a commit to halfline/uncrustify that referenced this issue Nov 18, 2022
This commit adds a new option, align_var_dev_star_dangle_gap, that
when paired with align_var_dev_star_style = dangle, provides a
way to specify a minimum gap that stars won't dangle into.

Closes: uncrustify#3770
@halfline halfline linked a pull request Nov 18, 2022 that will close this issue
halfline added a commit to halfline/uncrustify that referenced this issue Nov 18, 2022
This commit adds a new option, align_var_dev_star_dangle_gap, that
when paired with align_var_dev_star_style = dangle, provides a
way to specify a minimum gap that stars won't dangle into.

Closes: uncrustify#3770
@micheleCTDE
Copy link
Collaborator

micheleCTDE commented Nov 19, 2022

I did some tests here. The double space on the second function seems unrelated to the first function alignment. If you use nl_func_def_args = remove, you get this:

do_source_file(1524): Parsing: in.cpp as language CPP
static int
this_function_has_a_pointer_type (OtherType number, Object *object)
{
        return 0;
}

static int
this_function_has_no_pointer_type (int  number)
{
        return 0;
}

You can see you still have the double space in the second function anyway.

Further checking seems to indicate that the additional space comes from align_func_params_span and align_func_params_gap.
It seems to me there is a bug with align_func_params_span because even setting it to 2 still causes alignment in the second function. So rather than adding an additional option, I think we should first investigate why align_func_params_span is acting weird and fix its behavior.

Btw, you can use the option -ds dump -L A 2>log.txt to capture a lot more info helpful for debugging.

@halfline
Copy link
Contributor Author

halfline commented Nov 19, 2022

I'm not sure I understand the bug you're saying. My (very limited) understanding of things is:

align_func_params_span → the maximum number of lines to try aligning
align_func_params_gap → the minimum amount of space (and/or stars) between type name and parameter name

So I would expect align_func_params_span to have no bearing at all on a parameter list that is only one line. It's about how parameters in multiline parameter lists are aligned, right?

Furthermore, if align_func_params_gap is 2, I would expect there to always be at least two spaces between the type name and the variable name for parameters that have no stars.

It does seem weird to me that there is only one space between OtherType and number in your example above, if the align_func_params_gap is 2 but that seems like maybe a separate bug?

Let me know if my understanding of align_func_params_gap and align_func_params_span is off base.

Assuming they aren't off base, I don't think there is a way to express what I want with the current options. If you know of a set of options that is supposed to give me what I want but isn't, let me know, and I'll try to figure out why they aren't working.

@micheleCTDEAdmin
Copy link
Collaborator

micheleCTDEAdmin commented Nov 20, 2022

align_func_params_span → the maximum number of lines to try aligning

This is correct. The bug I am referring to is that it seems that align_func_params_span doesn't work properly. Your config use a value of 10, but even if you change it to 1, it seems to affect the alignment of the number in the second function call. If you set it to 0, you will see the second function is not affected. So this is the first thing to address, IMO.

align_func_params_gap → the minimum amount of space (and/or stars) between type name and parameter name

This should be the minimum amount of space between the type and the alignment column. Currently, IMO there is bug in the fact that this does not consider * and &. You are using align_var_def_star_style = 2 so I would expect the gap to be between the type and the star in you example. You can see that if you use align_var_def_star_style = 1, the line with the * moves, not the alignment of number in the first function. IMO, with align_var_def_star_style = 2, the output should be:

static int
this_function_has_a_pointer_type (OtherType   number,
                                  Object     *object)
{
        return 0;
}

where the 2-space gap is between the type and the left-most part of the alignment block, which is *. So it seems to me that the correct solution would be to fix the code for alignment of parameters, taking into considerations the value of align_var_def_star_style and similar for &.

Last, the code in PR #3889 seems to incorrectly affect the functionality of (the already broken) align_func_params_span. In the test example in the code, line 9 is not correctly aligned to line 2, even though the test is using align_func_params_span = 10, making line 9 and line 2 belonging to the same alignment stack.

Hope this helps to making things a bit clearer. IMO, the correct solution would be working on fixing point 1 and 2.

@micheleCTDEAdmin
Copy link
Collaborator

There may actually be another bug as well: I would expect number of the second function call to be aligned to number in the first function call, when they are within the same alignment stack. This does not seem to happen at all. I haven't looked into what the code does, but the behavior seems definitely NOT consistent with other alignment patterns, so I suspect it is badly broken.

@halfline
Copy link
Contributor Author

I believed align_func_params_span was a per-parameter list option. I didn't think it was supposed to align two parameter lists from two separate function definitions with each other if they happen to be within span lines of each other! That's very surprising to me. I mean usually functions are longer than the maximum span possible (which is 16 right) anyway?

I'll have to do some experimenting and get a better handle on what these configuration options actually mean

@halfline
Copy link
Contributor Author

halfline commented Nov 20, 2022

So looking at the code, I'm pretty sure the span is only supposed to be per parameter list.
I think the flow is roughly here:

22  Chunk *align_func_param(Chunk *start)•
│ 23  {•
…
│ 67     while ((pc = pc->GetNext())->IsNotNullChunk())•
│ 68     {•
…
│104        else if (pc->GetLevel() <= start->GetLevel())•
│105        {•
│106           break;•
│107        }•
│108        else if (pc->TestFlags(PCF_VAR_DEF))•
│109        {•
…
│122              many_as[pc->GetLevel()].Add(pc);•
…
│124        }•
…
│152     }•
…                                          
│156        for (size_t idx = 1; idx <= max_level_is; idx++)•
│157        {•
│158           many_as[idx].End();•
│159        }•
…
│162  } // align_func_param•

My understanding is that line 106 breaks out of the current aligning operation when the code gets to a chunk that is no longer in the parameter list (I'm assuming "level" is the nesting level of parenthesis here, but may be wrong)

@halfline
Copy link
Contributor Author

align_func_params_span → the maximum number of lines to try aligning

This is correct. The bug I am referring to is that it seems that align_func_params_span doesn't work properly. Your config use a value of 10, but even if you change it to 1, it seems to affect the alignment of the number in the second function call. If you set it to 0, you will see the second function is not affected. So this is the first thing to address, IMO.

So my understanding is that align_func_params_span = 0 is the same as saying "don't apply align_func_params_gap at all". In other words, the span is the maximum number of lines to try aligning. If the span is zero, no alignment will happen.

So i fully expect a span of 1, to apply a gap to the first line of the parameter list of each function, but a span of 0 to not apply a gap to any of the parameters in any of the parameter lists.

align_func_params_gap → the minimum amount of space (and/or stars) between type name and parameter name

This should be the minimum amount of space between the type and the alignment column. Currently, IMO there is bug in the fact that this does not consider * and &. You are using align_var_def_star_style = 2 so I would expect the gap to be between the type and the star in you example.

So now I'm confused again, I thought the whole point of align_var_def_star_style = 2 is that the code pretends like the star isn't there, and let's it invade the gap. I'll have to dig a little bit ...

@halfline
Copy link
Contributor Author

The docs say this:

# 0: Part of the type     'void *   foo;' (default)
# 1: Part of the variable 'void     *foo;'
# 2: Dangling             'void    *foo;'
# Dangling: the '*' will not be taken into account when aligning.

So it shows the star encroaching into the gap with align_var_def_star_style = 2 and it says that with that style the * will not be considered when aligning. So, assuming I'm not misunderstanding something (and I may very well be), I think it's expected that there may be less than align_func_params_gap spaces between a type name and * if align_var_def_star_style = 2

@micheleCTDE
Copy link
Collaborator

I believed align_func_params_span was a per-parameter list option. I didn't think it was supposed to align two parameter lists from two separate function definitions with each other if they happen to be within span lines of each other!

I have not looked at the code for this specific case (function parameter alignment) but I believe span and thres works like the other scenarios. At least, I would definitely expect so, since the names are the same. The idea as far as I can say, is to align parameters across multiple function declarations, which may be on consecutive lines. Not an option that I use, so I would have to do some testing too.

My understanding is that line 106 breaks out of the current aligning operation when the code gets to a chunk that is no longer in the parameter list (I'm assuming "level" is the nesting level of parenthesis here, but may be wrong)

You could be right and I could be wrong, I haven't really gone deeper in the code. Line 106 breaks out when we reach an outer level than the start (i.e. likely the ')' or the function.). Nevertheless alignment assumes we align across multiple lines, IMO, hence the span option.
@guy-maurel are you able to elaborate further on this point?

So my understanding is that align_func_params_span = 0 is the same as saying "don't apply align_func_params_gap at all". In other words, the span is the maximum number of lines to try aligning. If the span is zero, no alignment will happen.

Correct.

So i fully expect a span of 1, to apply a gap to the first line of the parameter list of each function, but a span of 0 to not apply a gap to any of the parameters in any of the parameter lists.

Span of X would mean looking at the next X lines and search for opportunity for alignment. Works like that for var definition, so I assume it should work like that for function parameters too.

So it shows the star encroaching into the gap with align_var_def_star_style = 2 and it says that with that style the * will not be considered when aligning.

Good finding, but it seems the comment is wrong and I was also wrong in my post yesterday. I did a quick test with

int *b;
int **c;
int *d;
int e;

and based on the value of align_var_def_star_style (0, 1 or 2), the alignment changes and it seems that stars are being considered too (or maybe the whole alignment stack gets shifted accordingly).

@micheleCTDE
Copy link
Collaborator

It definitely looks like we need 1) to do further investigation on what the behavior should be and 2) that there are a few things that may need some work on :-)

@halfline
Copy link
Contributor Author

So, tbh, I just did some tests, and it seems to be behaving like the comment for me. Maybe you have an outstanding patch or something? Or maybe our configs are subtly different...

Regardless, it would be good to have better test coverage in the test suite for testing a gap with the different star styles, so i'll do a pull request for that.

halfline added a commit to halfline/uncrustify that referenced this issue Nov 21, 2022
This commit adds a new option, align_var_dev_star_dangle_gap, that
when paired with align_var_dev_star_style = dangle, provides a
way to specify a minimum gap that stars won't dangle into.

Closes: uncrustify#3770
@halfline
Copy link
Contributor Author

halfline commented Nov 21, 2022

⬆️ okay that adds the tests to show the way it currently works. Once that gets merged I'll rebase #3889 on top of it and make it augment that test instead of adding a separate test.

Can you see if the test suite passes with these new tests on your machine?

@micheleCTDEAdmin
Copy link
Collaborator

@halfline
Thanks for the tests, going to merge them soon. So the align function parameter functionality is local to a function and aligns the parameters if they are on multiple lines.
I think we should also update the comment of the option to better explain this. For example, I had a totally different understanding, where I thought the option was for aligning function parameters across multiple function prototypes on consecutive lines. If it is ok for you, I will leave it to you to update the comment in a new PR.

@micheleCTDEAdmin
Copy link
Collaborator

Regarding the gap and star/ampersand, what do you think we should do? IMO it would be good if the gap was considering * and & when align_var_def_star_style is 2 (not in case of 0 or 1). Should we do that as a default? or have an option to let the user choose?

@halfline
Copy link
Contributor Author

halfline commented Nov 22, 2022

ah, yea I think there are separate options for aligning the parameter lists of function prototypes with each other. That's align_func_proto_* I believe.

I can do a pull request adding a more fleshed out comment, sure.

I personally don't see a way to make the current options do I what I want. For my use case, I need two separate gaps: I need a gap where stars are allowed to go, but variable names aren't allowed to go, and I need a gap where neither stars nor variable names are allowed to go.

I don't think there's a way we can implicitly guess when to allow a star into a gap and when to make it avoid the gap.

In my case, the gap where neither star nor variable name can go is only 1 column. We could hardcode that 1 column and I would be happy, but that sort of change is likely to break existing configs in the wild, and it might make other people unhappy. It's purely subjective whether the extra column is aesthetically pleasing or not.

So my next step will be to rebase #3889 on top of master. I'll make it modify the new test case source that just landed instead of adding its own and I'll throw in more docs in the same pull request, just for convenience sake, since it's all roughly related anyway.

EDIT: I actually ended up using a new test case source file anyway. It was just clearer to see the results that way.

halfline added a commit to halfline/uncrustify that referenced this issue Nov 22, 2022
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 won't dangle into.

Closes: uncrustify#3770
halfline added a commit to halfline/uncrustify that referenced this issue Nov 22, 2022
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 won't dangle into.

Closes: uncrustify#3770
halfline added a commit to halfline/uncrustify that referenced this issue Nov 22, 2022
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 won't dangle into.

Closes: uncrustify#3770
@micheleCTDEAdmin
Copy link
Collaborator

I personally don't see a way to make the current options do I what I want. For my use case, I need two separate gaps: I need a gap where stars are allowed to go, but variable names aren't allowed to go, and I need a gap where neither stars nor variable names are allowed to go.

Good use case! Will fit me too :-)
See comment on #3889 about whether to achieve that with 2 (or 3) options or just 1. We can further discuss there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants