[clang-format] Remove redundant test in style-on-command-line.cpp

[krasimir]

Summary

rL292562 added a fix to always format if the fallback style is set to "none".
In test/Format/style-on-command-line.cpp:19 is redundant, since -fallback-style
has a default value of LLVM set in ClangFormat.cpp:72.

@amaiorano: I believe that the rest of the test cases still cover your change in
case the fallback style is explicitly set to "none". Please, if this is not the
case, initiate a discussion.

[ioeric]

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

[amaiorano]

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

Of course I don't mind :) Why did it fail your internal tests, btw? Just curious. Was it something I could've detected myself?

[ioeric]

In D28943#651488, @amaiorano wrote:

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

Of course I don't mind :) Why did it fail your internal tests, btw? Just curious. Was it something I could've detected myself?

Probably not... it's just that our default fallback style is "Google" instead of "LLVM".

[amaiorano]

In D28943#651489, @ioeric wrote:

In D28943#651488, @amaiorano wrote:

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

Of course I don't mind :) Why did it fail your internal tests, btw? Just curious. Was it something I could've detected myself?

Probably not... it's just that our default fallback style is "Google" instead of "LLVM".

(I replied the following by email, but I'm not sure where it went... posting it here instead)

You mean you build a modified version of clang-format where Style is initialized to getGoogleStyle()?

I had wondered whether adding a "defaultStyle" argument might be useful, specifically in the case where you want to pass in yaml that simply tweaks the default style, but I figured it's not much harder to pass in "BasedOnStyle" in the yaml.

[ioeric]

In D28943#651536, @amaiorano wrote:

In D28943#651489, @ioeric wrote:

In D28943#651488, @amaiorano wrote:

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

Of course I don't mind :) Why did it fail your internal tests, btw? Just curious. Was it something I could've detected myself?

Probably not... it's just that our default fallback style is "Google" instead of "LLVM".

(I replied the following by email, but I'm not sure where it went... posting it here instead)

You mean you build a modified version of clang-format where Style is initialized to getGoogleStyle()?

I had wondered whether adding a "defaultStyle" argument might be useful, specifically in the case where you want to pass in yaml that simply tweaks the default style, but I figured it's not much harder to pass in "BasedOnStyle" in the yaml.

No.. we set both default style and default fallback style (tool options) to "google" in our build.

I don't see why "defaultStyle" would be useful. I think having the existing "style" and "fallback-style" options is sufficient (and already a bit confusing).

cfe/trunk/test/Format/style-on-command-line.cpp

 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK7 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, PointerBindsToType: true}" %s | FileCheck -strict-whitespace -check-prefix=CHECK8 %s
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
 
 // Fallback style tests
 // RUN: rm %T/_clang-format
 // Test no config file found, WebKit fallback style is applied
 // RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
-// Test no config file and no fallback style, LLVM style is applied
-// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 // Test no config file and fallback style "none", no formatting is applied
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 // Test config file with no based style, and fallback style "none", formatting is applied
 // RUN: printf "IndentWidth: 6\n" > %T/_clang-format
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
 // Test yaml with no based style, and fallback style "none", LLVM formatting applied
-// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK14 %s
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK13 %s
 
 void f() {
 // CHECK1: {{^        int\* i;$}}
 // CHECK2: {{^       int \*i;$}}
 // CHECK3: Unknown value for BasedOnStyle: invalid
 // CHECK3: Error parsing -style: {{I|i}}nvalid argument
 // CHECK4: Error parsing -style: {{I|i}}nvalid argument
 // CHECK5: {{^     int\* i;$}}
 // CHECK6: {{^Error reading .*\.clang-format: (I|i)nvalid argument}}
 // CHECK7: {{^      int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^    int \*i;$}}
 // CHECK10: {{^    int\* i;$}}
-// CHECK11: {{^  int \*i;$}}
-// CHECK12: {{^int\*i;$}}
-// CHECK13: {{^      int \*i;$}}
-// CHECK14: {{^       int \*i;$}}
+// CHECK11: {{^int\*i;$}}
+// CHECK12: {{^      int \*i;$}}
+// CHECK13: {{^       int \*i;$}}
 int*i;
 int j;
 }
 
 // On Windows, the 'rm' commands fail when the previous process is still alive.
 // This happens enough to make the test useless.
 // REQUIRES: shell