[PATCH] D115738: [clang-format] Fix formatting of the code that follows C# Lambda Expressions

[Peter Stys via Phabricator via cfe-commits]

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