[PATCH] D146042: [clang-format] Fix numerous issues with "LambdaBodyIndentation: OuterScope" option

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:21916
 
   // Lambdas with different indentation styles.
+  Style = getLLVMStyleWithColumns(60);
----------------
jp4a50 wrote:
> MyDeveloperDay wrote:
> > jp4a50 wrote:
> > > MyDeveloperDay wrote:
> > > > Why are you changing existing tests
> > > Sorry about the rather large diff here.
> > > 
> > > I have made the following changes to existing tests (all for what I think are good reasons):
> > > 
> > >   - Reduced the column limit and then reduced the verbosity of the code samples in the test to make them more readable. Before the column limit was 100 and the code samples were very long. I found this made them unnecessarily difficult to read, especially because some of the lines of the code sample would wrap before a newline in the code itself, which was confusing.
> > >   - Changed a number of tests written in the form `EXPECT_EQ(code, format(otherCode))` to simply `verifyFormat(code)` because the latter is much shorter and easier to read. If you can think of a good reason to favour the former over the latter then let me know but it just seemed like unnecessary duplication to me.
> > >   - Some of the tests weren't syntactically valid C++ e.g. the tests at line 21939 which I have changed to have a valid (rather than incomplete) trailing return type.
> > >   - The macro test actually captured a bug so I had to change the code sample there to reflect the now fixed behaviour.
> > > 
> > > I also added one or two more tests at the bottom to capture more complex cases like when more than one argument to a function call is a lambda. 
> > > 
> > > 
> > refactoring the tests is one thing, but not at the same time you are modifying how they are formatted. Could it be a separate commit so we can actually see what is changing?
> > 
> > I don't deny what is there before doesn't look 100% correct, but I've always worked on the Beyoncé rule  (If you liked it you should have put a test on it), meaning... someone put a test there to assert some form of formatting behaviour, you are now changing that to be your desired behaviour, that break the rule (IMHO)
> > 
> > we have to agree what was there before wasn't correct, but your changes don't give me that confidence, 
> > refactoring the tests is one thing, but not at the same time you are modifying how they are formatted. Could it be a separate commit so we can actually see what is changing?
> 
> Sure! I assume that this implies two separate Phabricator reviews/diffs? Or is there somehow a way to show two "commits" within a single review? Sorry, I'm not used to using Phrabricator.
> 
> > I don't deny what is there before doesn't look 100% correct, but I've always worked on the Beyoncé rule  (If you liked it you should have put a test on it), meaning... someone put a test there to assert some form of formatting behaviour, you are now changing that to be your desired behaviour, that break the rule (IMHO)
> 
> The macro example I was referring to as a "bug" actually had a comment below it which seemed to acknowledge an issue with the previous implementation. Now I'm not 100% sure that that comment was referring to the behaviour in the macro example but what does seem clear is that the behaviour shown in the macro test case is not desirable in the case of using `OuterScope`. I just can't see how that behaviour could be justified as being intentional/desirable. If you wanted, I could reach out to the original author of that test and ask them since they still work at the same company.
> 
> > we have to agree what was there before wasn't correct, but your changes don't give me that confidence, 
> 
> I'd appreciate if you could elaborate on this - perhaps you're referring mainly to the macro example but I'm not sure.
Each commit gets its own differential, there are no multiple commit reviews like pull requests. You can add a parent or child commit to show the dependency, if any.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146042/new/

https://reviews.llvm.org/D146042