benhamilton added inline comments.
================
Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+ Guesser.process();
+ if (Guesser.isObjC()) {
+ result = FormatStyle::LK_ObjC;
----------------
djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > In LLVM, we generally don't add braces for single statement ifs.
> > Mmmm.. is this a hard requirement? I've personally been bitten so many
> > times by adding statements after missing braces, I'd rather add them unless
> > they're required to not be there by the style guide.
> Yes. This is done as consistently as possible throughout LLVM and Clang and
> I'd like to keep clang-format's codebase consistent.
>
> clang-format itself makes it really hard to get bitten by missing braces :).
OK, will remove the braces for consistency. Maybe we should make clang-format
do this automatically?
================
Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+ if (Guesser.isObjC()) {
+ result = FormatStyle::LK_ObjC;
+ }
----------------
djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > Why not just return here?
> > I don't like early returns in case an else sneaks in later:
> >
> > https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
> But I would argue exactly the opposite. At this point, you have pretty
> uniquely determined that this is ObjC (from this originally being viewed as
> LK_Cpp). Now suppose you add additional logic before the return statement in
> line 2313: That would make the state space that this function can have quite
> complex.
>
> I would even go one step further and completely eliminate the variable
> "result". That would be slightly less efficient, so maybe I'd be ok with:
>
> const auto GuessFromFilename = getLanguageByFileName(FileName);
>
> And then you can return this at the end or have early exits / other code
> paths if you come up with different languages. Having both, a complex control
> flow and state in local variables seems unnecessarily complex here.
OK, happy to change it.
================
Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
+struct GuessLanguageTestCase {
+ const char *const FileName;
----------------
djasper wrote:
> benhamilton wrote:
> > djasper wrote:
> > > IMO, this is a bit over-engineered. IIUC, this only calls a single free
> > > standing function with two different parameters and expects a certain
> > > result. You don't need this struct, a test fixture or parameterized tests
> > > for that. Just write:
> > >
> > > TEST(GuessLanguageTest, FileAndCode) {
> > > EXPECT_EQ(guessLanguage("foo.cc", ""), FormatStyle::LK_Cpp);
> > > EXPECT_EQ(guessLanguage("foo.m", ""), FormatStyle::LK_ObjC);
> > > ...
> > > }
> > >
> > > Yes, you'd be duplicating the "EXPECT_EQ" and "guessLanguage", but I
> > > think that's actually good here. It makes the tests intuitively readable.
> > I hear you. I much prefer it when a single test tests a single issue, so
> > failures are isolated to the actual change:
> >
> > https://elgaard.blog/2011/02/06/multiple-asserts-in-a-single-unit-test-method/
> >
> > If this isn't a hard requirement, I'd like to keep this the way it is.
> It certainly makes sense for asserts, as a tests stops upon finding the first
> assert. But these are expectations. Each failing expectation will be reported
> individually, with a direct reference to the line in question and an easily
> understandable error message.
>
> I understand what you are saying but I think my proposal will actually make
> test failures easier to diagnose and understand. Please do change it.
Thanks, I appreciate the feedback and will make the change.
Repository:
rL LLVM
https://reviews.llvm.org/D43522
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
