djasper added inline comments.
================ Comment at: cfe/trunk/lib/Format/Format.cpp:2298 +FormatStyle::LanguageKind guessLanguage(StringRef FileName, StringRef Code) { + FormatStyle::LanguageKind result = getLanguageByFileName(FileName); + if (result == FormatStyle::LK_Cpp) { ---------------- Here and in several other places: Variables should be named with upper camel case (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). ================ Comment at: cfe/trunk/lib/Format/Format.cpp:2308 + Guesser.process(); + if (Guesser.isObjC()) { + result = FormatStyle::LK_ObjC; ---------------- In LLVM, we generally don't add braces for single statement ifs. ================ Comment at: cfe/trunk/lib/Format/Format.cpp:2309 + if (Guesser.isObjC()) { + result = FormatStyle::LK_ObjC; + } ---------------- Why not just return here? ================ Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955 +struct GuessLanguageTestCase { + const char *const FileName; ---------------- 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. 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