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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
