subject:"\[PATCH\] D28844\: clang\-format\: fix fallback style set to \"none\" not formatting"

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

2017-01-19 Thread Antonio Maiorano via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292562: clang-format: fix fallback style set to "none" not 
always formatting (authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D28844?vs=84795=85074#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28844

Files:
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/test/Format/style-on-command-line.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -1888,8 +1888,8 @@
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code,
- vfs::FileSystem *FS) {
+ StringRef FallbackStyleName,
+ StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
@@ -1903,9 +1903,9 @@
   (Code.contains("\n- (") || Code.contains("\n+ (")))
 Style.Language = FormatStyle::LK_ObjC;
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))
+return make_string_error("Invalid fallback style \"" + FallbackStyleName);
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.
@@ -1977,7 +1977,7 @@
 return make_string_error("Configuration file(s) do(es) not support " +
  getLanguageName(Style.Language) + ": " +
  UnsuitableConfigFiles);
-  return Style;
+  return FallbackStyle;
 }
 
 } // namespace format
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,31 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
+  // Test 2.3: format if config is found with no based style while fallback is
+  // 'none'.
+  ASSERT_TRUE(FS.addFile("/b/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("IndentWidth: 2")));
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
+  // Test 2.4: format if yaml with no based style, while fallback is 'none'.
+  Style2 = getStyle("{}", "a.h", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index: cfe/trunk/test/Format/style-on-command-line.cpp
===
--- cfe/trunk/test/Format/style-on-command-line.cpp
+++ cfe/trunk/test/Format/style-on-command-line.cpp
@@ -11,6 +11,21 @@
 // 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
+// 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

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

2017-01-17 Thread Daniel Jasper via Phabricator via cfe-commits

djasper accepted this revision.
djasper added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Format/Format.cpp:1906
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + 
FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))

amaiorano wrote:
> Technically, we don't need to initialize FallbackStyle to getNoStyle() here 
> as the call to getPredefinedStyle just below will initialize it. Let me know 
> what you would prefer here.
This seems fine.



Comment at: test/Format/style-on-command-line.cpp:15
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s

amaiorano wrote:
> Tests no config file found, WebKit fallback style is applied
Please put these into comments (as long as they don't start with "RUN", they 
should be just left alone as normal comments.


https://reviews.llvm.org/D28844



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

2017-01-17 Thread Antonio Maiorano via Phabricator via cfe-commits

amaiorano added inline comments.



Comment at: lib/Format/Format.cpp:1906
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + 
FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))

Technically, we don't need to initialize FallbackStyle to getNoStyle() here as 
the call to getPredefinedStyle just below will initialize it. Let me know what 
you would prefer here.



Comment at: test/Format/style-on-command-line.cpp:15
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s

Tests no config file found, WebKit fallback style is applied



Comment at: test/Format/style-on-command-line.cpp:16
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s

Tests no config file found, no fallback style, LLVM style is applied



Comment at: test/Format/style-on-command-line.cpp:17
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
 void f() {

Tests no config file found, fallback style set to "none", no formatting is 
applied


https://reviews.llvm.org/D28844



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

2017-01-17 Thread Antonio Maiorano via Phabricator via cfe-commits

amaiorano created this revision.

This change fixes the fact that fallback style set to "none" should not format. 
Without this change, fallback style "none" ends up applying LLVM formatting.


https://reviews.llvm.org/D28844

Files:
  lib/Format/Format.cpp
  test/Format/style-on-command-line.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,18 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int 
i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index: test/Format/style-on-command-line.cpp
===
--- test/Format/style-on-command-line.cpp
+++ test/Format/style-on-command-line.cpp
@@ -11,6 +11,10 @@
 // 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
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
 void f() {
 // CHECK1: {{^int\* i;$}}
 // CHECK2: {{^   int \*i;$}}
@@ -22,6 +26,9 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
+// CHECK10: {{^int\* i;$}}
+// CHECK11: {{^  int \*i;$}}
+// CHECK12: {{^int\*i;$}}
 int*i;
 int j;
 }
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1888,8 +1888,8 @@
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code,
- vfs::FileSystem *FS) {
+ StringRef FallbackStyleName,
+ StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
@@ -1903,9 +1903,9 @@
   (Code.contains("\n- (") || Code.contains("\n+ (")))
 Style.Language = FormatStyle::LK_ObjC;
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + 
FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))
+return make_string_error("Invalid fallback style \"" + FallbackStyleName);
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.
@@ -1977,7 +1977,7 @@
 return make_string_error("Configuration file(s) do(es) not support " +
  getLanguageName(Style.Language) + ": " +
  UnsuitableConfigFiles);
-  return Style;
+  return FallbackStyle;
 }
 
 } // namespace format


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,18 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index:

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

4 matches

Site Navigation

Mail list logo

Footer information