HazardyKnusperkeks marked an inline comment as done. HazardyKnusperkeks added a comment.
In D92257#2452063 <https://reviews.llvm.org/D92257#2452063>, @MyDeveloperDay wrote: > This didn't really address the comments, what is the point of the maximum? > what if the maximum is > the ColumnLimit? Current behavior if there are more spaces than ColumnLimit: Do not format the comment at all. Now if the minimum is larger than the ColumnLimit we will obey the limit and then normal behavior kicks in, this is also covered in the tests. ================ Comment at: clang/docs/ClangFormatStyleOptions.rst:2727 + + * ``unsigned Maximum`` The maximum number of spaces at the start of the comment. + ---------------- HazardyKnusperkeks wrote: > krasimir wrote: > > HazardyKnusperkeks wrote: > > > MyDeveloperDay wrote: > > > > I'm personally not a massive fan of stuffing -1 into an unsigned but I > > > > understand why its there, if this was an signed and it was actually -1 > > > > would the algorithm be substantially worse in your view? > > > I'm no fan if unsigned in any way, but that seems to be the way in > > > clang-format. > > > Making it signed would require a few more checks when to use it, but I > > > don't see any problem in that. > > > I just would also make the Minimum signed then just to be consistent. > > > > > > While parsing the style I would add checks to ensure Minimum is never > > > negative and Maximum is always greater or equal to -1, should that print > > > any warnings? Is there a standard way of doing so? Or should it be just > > > silently corrected? > > I find it confusing why we have 2, Minimum and Maximum, instead of a single > > one. > > I'm not convinced that `Maximum` is useful. > > Conceptually I'd prefer a single integer option, say > > `LineCommentContentIndent` that would indicate the default indent used for > > the content of line comments. I'd naively expect `LineCommentContentIndent > > = `: > > * 0 would produce `//comment` > > * 1 would produce `// comment` (current default) > > * 2 would produce `// comment`, etc. > > and this will work with if the input is any of `//comment`, `// comment`, > > or `// comment`, etc. > > > > An additional consideration is that line comment sections often contain > > additional indentation, e.g. when there is a bullet list, paragraphs, etc. > > and so we can't guarantee that the indent of each line comment will be less > > than Maximum in general. I'd expect this feature to not adjust extra indent > > in comments, e.g., > > ``` > > // Lorem ipsum dolor sit amet, > > // consectetur adipiscing elit, > > // ... > > ``` > > after reformatting with `LineCommentContentIndent=0` to produce > > ``` > > //Lorem ipsum dolor sit amet, > > // consectetur adipiscing elit, > > // ... > > ``` > > (and vice-versa, after reformatting with `LineCommentContentIndent=1`). > > This may well be handled by code, I just wasn't sure by looking at the code > > and test examples. > I was actually going for only one value, but while writing the tests I came > to the conclusion that before my change is only enforced a minimum of 1. But > that very well may be because of what you call line comment sections, I did > not consider that. That's why I chose a minimum and maximum. > > I will modify the patch to one value and will also add tests for the > sections. But for that I need to remember if I added or removed spaces, > right? Is there already infrastructure for that? Or is there any > documentation of the various steps clang-format takes to parse and format > code? Until now I tried to understand what's going on through single stepping > with the debugger (quite time consuming). > I find it confusing why we have 2, Minimum and Maximum, instead of a single > one. > I'm not convinced that `Maximum` is useful. > Conceptually I'd prefer a single integer option, say > `LineCommentContentIndent` that would indicate the default indent used for > the content of line comments. I'd naively expect `LineCommentContentIndent = > `: > * 0 would produce `//comment` > * 1 would produce `// comment` (current default) > * 2 would produce `// comment`, etc. > and this will work with if the input is any of `//comment`, `// comment`, or > `// comment`, etc. > > An additional consideration is that line comment sections often contain > additional indentation, e.g. when there is a bullet list, paragraphs, etc. > and so we can't guarantee that the indent of each line comment will be less > than Maximum in general. I'd expect this feature to not adjust extra indent > in comments, e.g., > ``` > // Lorem ipsum dolor sit amet, > // consectetur adipiscing elit, > // ... > ``` > after reformatting with `LineCommentContentIndent=0` to produce > ``` > //Lorem ipsum dolor sit amet, > // consectetur adipiscing elit, > // ... > ``` > (and vice-versa, after reformatting with `LineCommentContentIndent=1`). > This may well be handled by code, I just wasn't sure by looking at the code > and test examples. ================ Comment at: clang/unittests/Format/FormatTestComments.cpp:3141-3147 " # commen3\n" "# commen4\n" "a: 1 # commen5\n" " # commen6\n" " # commen7", format("k:val#commen1 commen2\n" " # commen3\n" ---------------- Here the test fails, because `commen1` gets a space added and `commen3` belongs to the same section, thus also gets an additional space. I see three options: # The whole keeping indentation in a section is wrong. # Disable the mechanic for text proto. # Adapt the test. ================ Comment at: clang/unittests/Format/FormatTestComments.cpp:3399 + " // return ret2;\n" + " // }\n" + "\n" ---------------- Here is the difference. Before this would have been formatted as ``` // if (ret1) { // return 2; //} ``` So only one space added for the `if`, it did not keep the indentation of the `return` and not adding a space to `}`. I think this is much better and also basically what @krasimir requested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92257/new/ https://reviews.llvm.org/D92257 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
