HazardyKnusperkeks marked 6 inline comments as done.
HazardyKnusperkeks added a comment.

In D92257#2435906 <https://reviews.llvm.org/D92257#2435906>, @lebedev.ri wrote:

> In D92257#2435902 <https://reviews.llvm.org/D92257#2435902>, @MyDeveloperDay 
> wrote:
>
>> In D92257#2435899 <https://reviews.llvm.org/D92257#2435899>, 
>> @HazardyKnusperkeks wrote:
>>
>>> In D92257#2435701 <https://reviews.llvm.org/D92257#2435701>, 
>>> @MyDeveloperDay wrote:
>>>
>>>> Can I assume you need someone to land this for you?
>>>
>>> Yes I do. But I have a question, my last change is commited in your name, 
>>> that means git blame would blame it on you, right?
>>>
>>> You can set me as author:
>>> `Björn Schäpers <bjo...@hazardy.de>`
>>> My Github Account is also called `HazardyKnusperkeks`.
>>
>> The process is that you add (https://llvm.org/docs/Contributing.html)
>>
>> Patch By: HazardyKnusperkeks
>>
>> to the commit message if the user doesn't have commit access, if you want 
>> your name against the blame then I recommend applying for commit access 
>> yourself.
>
> That is incorrect and does not represent the nowadays reality, i suggest that 
> you look up the docs.
>
>> let me know if you still want me to land this

What/Where are the docs? I read https://llvm.org/docs/Contributing.html before 
hand and just now https://llvm.org/docs/CodeReview.html

---

I will update the patch, but that won't happen before the weekend.



================
Comment at: clang/docs/ClangFormatStyleOptions.rst:2727
+
+  * ``unsigned Maximum`` The maximum number of spaces at the start of the 
comment.
+
----------------
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).


================
Comment at: clang/lib/Format/BreakableToken.cpp:790
+          (Style.Language != FormatStyle::LK_TextProto ||
+           OriginalPrefix[i].substr(0, 2) != "##")) {
+        Prefix[i] = IndentPrefix.str();
----------------
krasimir wrote:
> HazardyKnusperkeks wrote:
> > HazardyKnusperkeks wrote:
> > > MyDeveloperDay wrote:
> > > > is this case covered by a unit test at all? sorry can you explain why 
> > > > you are looking for "##"?
> > > It is covered by multiple tests, that's how I was made aware of it. :)
> > > If you look at the code before it only adds a space if the old prefix is 
> > > "#" not "##" which is also found by `getLineCommentIndentPrefix`. As it 
> > > seems in `TextProto` "##" should not be touched. I can of course add a 
> > > test in my test function.
> > > 
> > > Now I see a change, in the code before "#" was only accepted when the 
> > > language is `TextProto`, now it is always. But I think for that to happen 
> > > the parser (or lexer?) should have assigned something starting with"#" as 
> > > comment, right? But I can change that.
> > Okay # # is formatted, I try again:
> > If you look at the code before it only adds a space if the old prefix is 
> > "#" not "`##`" which is also found by `getLineCommentIndentPrefix`. As it 
> > seems in `TextProto` "`##`" should not be touched.
> Thanks for the analysis!
> I wrote the text proto comment detection. I believe the current clang-format 
> is buggy in that it should transform `##comment` into `## comment` for text 
> proto (and similarly for all other `KnownTextProtoPrefixes` in 
> `getLineCommentIndentCommentPrefix`), so this `substr(0, 2) != "##"` is 
> unnecessary and I should go ahead and update and add tests for that.
In that case I will remove that check, but as said there were many tests which 
failed without it, I will have to adapt them too.


================
Comment at: clang/unittests/Format/FormatTestComments.cpp:3405
+            "//  Lorem   ipsum\n"
+            "//  dolor   sit amet\n" // Why are here the spaces dropped?
+            "\n"
----------------
krasimir wrote:
> This is desired, AFAIK, and due to the normalization behavior while 
> reflowing: when a comment line exceeds the comment limit and is broken up 
> into a new line, the full range of blanks is replaced with a newline. 
> (https://github.com/llvm/llvm-project/blob/ddb002d7c74c038b64dd9d3c3e4a4b58795cf1a6/clang/lib/Format/BreakableToken.cpp#L66).
> Note that reflowing copies the extra indent of the line, e.g.,
> ```
> // line limit  V
> // heading
> // *line is
> //   long long long long 
> ```
> get reformatted as
> ```
> // line limit  V
> // heading
> // *line is
> //   long long
> //   long long 
> ```
> so if for ranges of blanks longer of size S>1 we copied the (S-1) blanks at 
> the beginning of the next line, we would have cascading comment reflows 
> undesired with longer and longer indents.
> 
Okay, I mean the spaced between `sit` and `amet`, while the spaces between 
`Lorem` and `ipsum`, and `dolor` and `sit` is kept.


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

Reply via email to