jp4a50 added inline comments.
================
Comment at: clang/unittests/Format/FormatTest.cpp:21952
"}",
Style);
+ verifyFormat("void test() {\n"
----------------
MyDeveloperDay wrote:
> I feel like lots of these test changes could be made on their own, BEFORE
> changing the code, I would feel more comfortable about that I think
Pulled them out into D146995.
================
Comment at: clang/unittests/Format/FormatTest.cpp:21916
// Lambdas with different indentation styles.
+ Style = getLLVMStyleWithColumns(60);
----------------
HazardyKnusperkeks wrote:
> 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.
Pulled out test refactoring (excluding functional changes) into D146995.
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
