[PATCH] D43522: [clang-format] New API guessLanguage()

[Daniel Jasper via Phabricator via llvm-commits]

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