benhamilton 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) {
----------------
djasper wrote:
> 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).
Thanks, will send a follow-up.
================
Comment at: cfe/trunk/lib/Format/Format.cpp:2308
+ Guesser.process();
+ if (Guesser.isObjC()) {
+ result = FormatStyle::LK_ObjC;
----------------
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.
================
Comment at: cfe/trunk/lib/Format/Format.cpp:2309
+ if (Guesser.isObjC()) {
+ result = FormatStyle::LK_ObjC;
+ }
----------------
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
================
Comment at: cfe/trunk/unittests/Format/FormatTest.cpp:11955
+struct GuessLanguageTestCase {
+ const char *const FileName;
----------------
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.
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
