peterstys marked 4 inline comments as done.
peterstys added inline comments.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1870
// C# may break after => if the next character is a newline.
if (Style.isCSharp() && Style.BraceWrapping.AfterFunction == true) {
// calling `addUnwrappedLine()` here causes odd parsing errors.
----------------
owenpan wrote:
> Let's fix this too while we are at it. In the future, we may want to factor
> lines 1862-1874 and 2004-2009.
I extracted the C# lambda parsing block into a separate function to avoid
repeating the same logic twice. Hope that is alright.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1872
// calling `addUnwrappedLine()` here causes odd parsing errors.
FormatTok->MustBreakBefore = true;
}
----------------
MyDeveloperDay wrote:
> part of me thinks the MustBreakBefore should be handled separately in
> TokenAnnotator::mustBreakBefore() and let it do its stuff.
I used MustBreakBefore = true because the same method was used for the FatArrow
processing on the lines 1862-1874 and it did the right thing i.e. added a new
line before the block.
If you want me to change the code to use TokenAnnotator::mustBreakBefore()
please provide more details on how to do it, or point me to some examples. I
failed to find any good references, I'm afraid.
================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1872
// calling `addUnwrappedLine()` here causes odd parsing errors.
FormatTok->MustBreakBefore = true;
}
----------------
owenpan wrote:
> peterstys wrote:
> > MyDeveloperDay wrote:
> > > part of me thinks the MustBreakBefore should be handled separately in
> > > TokenAnnotator::mustBreakBefore() and let it do its stuff.
> > I used MustBreakBefore = true because the same method was used for the
> > FatArrow processing on the lines 1862-1874 and it did the right thing i.e.
> > added a new line before the block.
> >
> > If you want me to change the code to use TokenAnnotator::mustBreakBefore()
> > please provide more details on how to do it, or point me to some examples.
> > I failed to find any good references, I'm afraid.
> Yes, but we can take care of it in a separate patch.
ACK
================
Comment at: clang/unittests/Format/FormatTestCSharp.cpp:766
+
+ verifyFormat(R"(//
+public class Sample {
----------------
MyDeveloperDay wrote:
> Nit: (only my preference) but I don't like the use of RawStrings in these
> tests (I know others have let them creep in) but I find them more unreadable
> because we lose the indentation.. I think I'm just so used to the other style
> that this just crates a little.
>
> Just my personal preference (you can ignore)
I used the RawStrings as it was very easy to copy snippet of code to a separate
file so I could run clang-format on it to check all was working well. I also
copied snippets to my local IDE to check if the file was building, or at least
structurally okay (which I appreciate is not a requirements as clang-format
does not build a complete AST of the code).
For example, I was surprised to see some sample code in unit tests missing
braces after catch (Exception) statement (line 694) (which may have been
intended). I find it slightly more difficult to read those code snippets if
they are decorated with \n.
On the other hand, you're right, raw string break the indentation which is not
great. Also, I'd rather follow the leading preferences (and wasn't sure which
ones where they before your comment). I'm happy to update this to your style if
that is the preference.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115738/new/
https://reviews.llvm.org/D115738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
