Skip to content

Commit 3adfb6a

Browse files
amaioranoamaiorano
committed
clang-format: Make GetStyle return Expected<FormatStyle> instead of FormatStyle
Change the contract of GetStyle so that it returns an error when an error occurs (i.e. when it writes to stderr), and only returns the fallback style when it can't find a configuration file. Differential Revision: https://reviews.llvm.org/D28081 llvm-svn: 292174
1 parent 2aab1d4 commit 3adfb6a

File tree

7 files changed

+110
-69
lines changed

7 files changed

+110
-69
lines changed

‎clang/include/clang/Format/Format.h

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -853,17 +853,19 @@ extern const char *StyleOptionHelpDescription;
853853
/// \param[in] FileName Path to start search for .clang-format if ``StyleName``
854854
/// == "file".
855855
/// \param[in] FallbackStyle The name of a predefined style used to fallback to
856-
/// in case the style can't be determined from \p StyleName.
856+
/// in case \p StyleName is "file" and no file can be found.
857857
/// \param[in] Code The actual code to be formatted. Used to determine the
858858
/// language if the filename isn't sufficient.
859859
/// \param[in] FS The underlying file system, in which the file resides. By
860860
/// default, the file system is the real file system.
861861
///
862-
/// \returns FormatStyle as specified by ``StyleName``. If no style could be
863-
/// determined, the default is LLVM Style (see ``getLLVMStyle()``).
864-
FormatStyle getStyle(StringRef StyleName, StringRef FileName,
865-
StringRef FallbackStyle, StringRef Code = "",
866-
vfs::FileSystem *FS = nullptr);
862+
/// \returns FormatStyle as specified by ``StyleName``. If ``StyleName`` is
863+
/// "file" and no file is found, returns ``FallbackStyle``. If no style could be
864+
/// determined, returns an Error.
865+
llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
866+
StringRef FallbackStyle,
867+
StringRef Code = "",
868+
vfs::FileSystem *FS = nullptr);
867869

868870
// \brief Returns a string representation of ``Language``.
869871
inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {

‎clang/lib/Format/Format.cpp

Lines changed: 29 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,11 @@ std::error_code make_error_code(ParseError e) {
421421
return std::error_code(static_cast<int>(e), getParseCategory());
422422
}
423423

424+
inline llvm::Error make_string_error(const llvm::Twine &Message) {
425+
return llvm::make_error<llvm::StringError>(Message,
426+
llvm::inconvertibleErrorCode());
427+
}
428+
424429
const char *ParseErrorCategory::name() const noexcept {
425430
return "clang-format.parse_error";
426431
}
@@ -1882,9 +1887,9 @@ static FormatStyle::LanguageKind getLanguageByFileName(StringRef FileName) {
18821887
return FormatStyle::LK_Cpp;
18831888
}
18841889

1885-
FormatStyle getStyle(StringRef StyleName, StringRef FileName,
1886-
StringRef FallbackStyle, StringRef Code,
1887-
vfs::FileSystem *FS) {
1890+
llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
1891+
StringRef FallbackStyle, StringRef Code,
1892+
vfs::FileSystem *FS) {
18881893
if (!FS) {
18891894
FS = vfs::getRealFileSystem().get();
18901895
}
@@ -1898,35 +1903,28 @@ FormatStyle getStyle(StringRef StyleName, StringRef FileName,
18981903
(Code.contains("\n- (") || Code.contains("\n+ (")))
18991904
Style.Language = FormatStyle::LK_ObjC;
19001905

1901-
if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style)) {
1902-
llvm::errs() << "Invalid fallback style \"" << FallbackStyle
1903-
<< "\" using LLVM style\n";
1904-
return Style;
1905-
}
1906+
// FIXME: If FallbackStyle is explicitly "none", format is disabled.
1907+
if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style))
1908+
return make_string_error("Invalid fallback style \"" + FallbackStyle.str());
19061909

19071910
if (StyleName.startswith("{")) {
19081911
// Parse YAML/JSON style from the command line.
1909-
if (std::error_code ec = parseConfiguration(StyleName, &Style)) {
1910-
llvm::errs() << "Error parsing -style: " << ec.message() << ", using "
1911-
<< FallbackStyle << " style\n";
1912-
}
1912+
if (std::error_code ec = parseConfiguration(StyleName, &Style))
1913+
return make_string_error("Error parsing -style: " + ec.message());
19131914
return Style;
19141915
}
19151916

19161917
if (!StyleName.equals_lower("file")) {
19171918
if (!getPredefinedStyle(StyleName, Style.Language, &Style))
1918-
llvm::errs() << "Invalid value for -style, using " << FallbackStyle
1919-
<< " style\n";
1919+
return make_string_error("Invalid value for -style");
19201920
return Style;
19211921
}
19221922

19231923
// Look for .clang-format/_clang-format file in the file's parent directories.
19241924
SmallString<128> UnsuitableConfigFiles;
19251925
SmallString<128> Path(FileName);
1926-
if (std::error_code EC = FS->makeAbsolute(Path)) {
1927-
llvm::errs() << EC.message() << "\n";
1928-
return Style;
1929-
}
1926+
if (std::error_code EC = FS->makeAbsolute(Path))
1927+
return make_string_error(EC.message());
19301928

19311929
for (StringRef Directory = Path; !Directory.empty();
19321930
Directory = llvm::sys::path::parent_path(Directory)) {
@@ -1943,25 +1941,23 @@ FormatStyle getStyle(StringRef StyleName, StringRef FileName,
19431941
DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
19441942

19451943
Status = FS->status(ConfigFile.str());
1946-
bool IsFile =
1944+
bool FoundConfigFile =
19471945
Status && (Status->getType() == llvm::sys::fs::file_type::regular_file);
1948-
if (!IsFile) {
1946+
if (!FoundConfigFile) {
19491947
// Try _clang-format too, since dotfiles are not commonly used on Windows.
19501948
ConfigFile = Directory;
19511949
llvm::sys::path::append(ConfigFile, "_clang-format");
19521950
DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
19531951
Status = FS->status(ConfigFile.str());
1954-
IsFile = Status &&
1955-
(Status->getType() == llvm::sys::fs::file_type::regular_file);
1952+
FoundConfigFile = Status && (Status->getType() ==
1953+
llvm::sys::fs::file_type::regular_file);
19561954
}
19571955

1958-
if (IsFile) {
1956+
if (FoundConfigFile) {
19591957
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
19601958
FS->getBufferForFile(ConfigFile.str());
1961-
if (std::error_code EC = Text.getError()) {
1962-
llvm::errs() << EC.message() << "\n";
1963-
break;
1964-
}
1959+
if (std::error_code EC = Text.getError())
1960+
return make_string_error(EC.message());
19651961
if (std::error_code ec =
19661962
parseConfiguration(Text.get()->getBuffer(), &Style)) {
19671963
if (ec == ParseError::Unsuitable) {
@@ -1970,19 +1966,17 @@ FormatStyle getStyle(StringRef StyleName, StringRef FileName,
19701966
UnsuitableConfigFiles.append(ConfigFile);
19711967
continue;
19721968
}
1973-
llvm::errs() << "Error reading " << ConfigFile << ": " << ec.message()
1974-
<< "\n";
1975-
break;
1969+
return make_string_error("Error reading " + ConfigFile + ": " +
1970+
ec.message());
19761971
}
19771972
DEBUG(llvm::dbgs() << "Using configuration file " << ConfigFile << "\n");
19781973
return Style;
19791974
}
19801975
}
1981-
if (!UnsuitableConfigFiles.empty()) {
1982-
llvm::errs() << "Configuration file(s) do(es) not support "
1983-
<< getLanguageName(Style.Language) << ": "
1984-
<< UnsuitableConfigFiles << "\n";
1985-
}
1976+
if (!UnsuitableConfigFiles.empty())
1977+
return make_string_error("Configuration file(s) do(es) not support " +
1978+
getLanguageName(Style.Language) + ": " +
1979+
UnsuitableConfigFiles);
19861980
return Style;
19871981
}
19881982

‎clang/lib/Tooling/Refactoring.cpp

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ int RefactoringTool::saveRewrittenFiles(Rewriter &Rewrite) {
6868
}
6969

7070
bool formatAndApplyAllReplacements(
71-
const std::map<std::string, Replacements> &FileToReplaces, Rewriter &Rewrite,
72-
StringRef Style) {
71+
const std::map<std::string, Replacements> &FileToReplaces,
72+
Rewriter &Rewrite, StringRef Style) {
7373
SourceManager &SM = Rewrite.getSourceMgr();
7474
FileManager &Files = SM.getFileManager();
7575

@@ -83,9 +83,14 @@ bool formatAndApplyAllReplacements(
8383
FileID ID = SM.getOrCreateFileID(Entry, SrcMgr::C_User);
8484
StringRef Code = SM.getBufferData(ID);
8585

86-
format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
86+
auto CurStyle = format::getStyle(Style, FilePath, "LLVM");
87+
if (!CurStyle) {
88+
llvm::errs() << llvm::toString(CurStyle.takeError()) << "\n";
89+
return false;
90+
}
91+
8792
auto NewReplacements =
88-
format::formatReplacements(Code, CurReplaces, CurStyle);
93+
format::formatReplacements(Code, CurReplaces, *CurStyle);
8994
if (!NewReplacements) {
9095
llvm::errs() << llvm::toString(NewReplacements.takeError()) << "\n";
9196
return false;

‎clang/test/Format/style-on-command-line.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// RUN: clang-format -style="{BasedOnStyle: Google, IndentWidth: 8}" %s | FileCheck -strict-whitespace -check-prefix=CHECK1 %s
22
// RUN: clang-format -style="{BasedOnStyle: LLVM, IndentWidth: 7}" %s | FileCheck -strict-whitespace -check-prefix=CHECK2 %s
3-
// RUN: clang-format -style="{BasedOnStyle: invalid, IndentWidth: 7}" -fallback-style=LLVM %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK3 %s
4-
// RUN: clang-format -style="{lsjd}" %s -fallback-style=LLVM 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK4 %s
3+
// RUN: not clang-format -style="{BasedOnStyle: invalid, IndentWidth: 7}" -fallback-style=LLVM %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK3 %s
4+
// RUN: not clang-format -style="{lsjd}" %s -fallback-style=LLVM 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK4 %s
55
// RUN: printf "BasedOnStyle: google\nIndentWidth: 5\n" > %T/.clang-format
66
// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK5 %s
77
// RUN: printf "\n" > %T/.clang-format
8-
// RUN: clang-format -style=file -fallback-style=webkit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK6 %s
8+
// RUN: not clang-format -style=file -fallback-style=webkit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK6 %s
99
// RUN: rm %T/.clang-format
1010
// RUN: printf "BasedOnStyle: google\nIndentWidth: 6\n" > %T/_clang-format
1111
// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK7 %s
@@ -15,13 +15,10 @@ void f() {
1515
// CHECK1: {{^ int\* i;$}}
1616
// CHECK2: {{^ int \*i;$}}
1717
// CHECK3: Unknown value for BasedOnStyle: invalid
18-
// CHECK3: Error parsing -style: {{I|i}}nvalid argument, using LLVM style
19-
// CHECK3: {{^ int \*i;$}}
20-
// CHECK4: Error parsing -style: {{I|i}}nvalid argument, using LLVM style
21-
// CHECK4: {{^ int \*i;$}}
18+
// CHECK3: Error parsing -style: {{I|i}}nvalid argument
19+
// CHECK4: Error parsing -style: {{I|i}}nvalid argument
2220
// CHECK5: {{^ int\* i;$}}
2321
// CHECK6: {{^Error reading .*\.clang-format: (I|i)nvalid argument}}
24-
// CHECK6: {{^ int\* i;$}}
2522
// CHECK7: {{^ int\* i;$}}
2623
// CHECK8: {{^ int\* i;$}}
2724
// CHECK9: {{^ int \*i;$}}

‎clang/tools/clang-format/ClangFormat.cpp

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -249,12 +249,17 @@ static bool format(StringRef FileName) {
249249
if (fillRanges(Code.get(), Ranges))
250250
return true;
251251
StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
252-
FormatStyle FormatStyle =
252+
253+
llvm::Expected<FormatStyle> FormatStyle =
253254
getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
255+
if (!FormatStyle) {
256+
llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
257+
return true;
258+
}
254259
if (SortIncludes.getNumOccurrences() != 0)
255-
FormatStyle.SortIncludes = SortIncludes;
260+
FormatStyle->SortIncludes = SortIncludes;
256261
unsigned CursorPosition = Cursor;
257-
Replacements Replaces = sortIncludes(FormatStyle, Code->getBuffer(), Ranges,
262+
Replacements Replaces = sortIncludes(*FormatStyle, Code->getBuffer(), Ranges,
258263
AssumedFileName, &CursorPosition);
259264
auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), Replaces);
260265
if (!ChangedCode) {
@@ -264,7 +269,7 @@ static bool format(StringRef FileName) {
264269
// Get new affected ranges after sorting `#includes`.
265270
Ranges = tooling::calculateRangesAfterReplacements(Replaces, Ranges);
266271
bool IncompleteFormat = false;
267-
Replacements FormatChanges = reformat(FormatStyle, *ChangedCode, Ranges,
272+
Replacements FormatChanges = reformat(*FormatStyle, *ChangedCode, Ranges,
268273
AssumedFileName, &IncompleteFormat);
269274
Replaces = Replaces.merge(FormatChanges);
270275
if (OutputXML) {
@@ -334,10 +339,15 @@ int main(int argc, const char **argv) {
334339
cl::PrintHelpMessage();
335340

336341
if (DumpConfig) {
337-
std::string Config =
338-
clang::format::configurationAsText(clang::format::getStyle(
342+
llvm::Expected<clang::format::FormatStyle> FormatStyle =
343+
clang::format::getStyle(
339344
Style, FileNames.empty() ? AssumeFileName : FileNames[0],
340-
FallbackStyle));
345+
FallbackStyle);
346+
if (!FormatStyle) {
347+
llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
348+
return 1;
349+
}
350+
std::string Config = clang::format::configurationAsText(*FormatStyle);
341351
outs() << Config << "\n";
342352
return 0;
343353
}

‎clang/unittests/Format/FormatTest.cpp

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10975,13 +10975,15 @@ TEST(FormatStyle, GetStyleOfFile) {
1097510975
ASSERT_TRUE(
1097610976
FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
1097710977
auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", &FS);
10978-
ASSERT_EQ(Style1, getLLVMStyle());
10978+
ASSERT_TRUE((bool)Style1);
10979+
ASSERT_EQ(*Style1, getLLVMStyle());
1097910980

1098010981
// Test 2: fallback to default.
1098110982
ASSERT_TRUE(
1098210983
FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
1098310984
auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", &FS);
10984-
ASSERT_EQ(Style2, getMozillaStyle());
10985+
ASSERT_TRUE((bool)Style2);
10986+
ASSERT_EQ(*Style2, getMozillaStyle());
1098510987

1098610988
// Test 3: format file in parent directory.
1098710989
ASSERT_TRUE(
@@ -10990,7 +10992,34 @@ TEST(FormatStyle, GetStyleOfFile) {
1099010992
ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
1099110993
llvm::MemoryBuffer::getMemBuffer("int i;")));
1099210994
auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", &FS);
10993-
ASSERT_EQ(Style3, getGoogleStyle());
10995+
ASSERT_TRUE((bool)Style3);
10996+
ASSERT_EQ(*Style3, getGoogleStyle());
10997+
10998+
// Test 4: error on invalid fallback style
10999+
auto Style4 = getStyle("file", "a.h", "KungFu", "", &FS);
11000+
ASSERT_FALSE((bool)Style4);
11001+
llvm::consumeError(Style4.takeError());
11002+
11003+
// Test 5: error on invalid yaml on command line
11004+
auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", &FS);
11005+
ASSERT_FALSE((bool)Style5);
11006+
llvm::consumeError(Style5.takeError());
11007+
11008+
// Test 6: error on invalid style
11009+
auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", &FS);
11010+
ASSERT_FALSE((bool)Style6);
11011+
llvm::consumeError(Style6.takeError());
11012+
11013+
// Test 7: found config file, error on parsing it
11014+
ASSERT_TRUE(
11015+
FS.addFile("/d/.clang-format", 0,
11016+
llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
11017+
"InvalidKey: InvalidValue")));
11018+
ASSERT_TRUE(
11019+
FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
11020+
auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS);
11021+
ASSERT_FALSE((bool)Style7);
11022+
llvm::consumeError(Style7.takeError());
1099411023
}
1099511024

1099611025
TEST_F(ReplacementTest, FormatCodeAfterReplacements) {

‎clang/unittests/Format/FormatTestObjC.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,21 @@ class FormatTestObjC : public ::testing::Test {
6868
FormatStyle Style;
6969
};
7070

71-
TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
72-
Style = getStyle("LLVM", "a.h", "none", "@interface\n"
73-
"- (id)init;");
74-
EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
71+
TEST(FormatTestObjCStyle, DetectsObjCInHeaders) {
72+
auto Style = getStyle("LLVM", "a.h", "none", "@interface\n"
73+
"- (id)init;");
74+
ASSERT_TRUE((bool)Style);
75+
EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
76+
7577
Style = getStyle("LLVM", "a.h", "none", "@interface\n"
7678
"+ (id)init;");
77-
EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
79+
ASSERT_TRUE((bool)Style);
80+
EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
7881

7982
// No recognizable ObjC.
8083
Style = getStyle("LLVM", "a.h", "none", "void f() {}");
81-
EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
84+
ASSERT_TRUE((bool)Style);
85+
EXPECT_EQ(FormatStyle::LK_Cpp, Style->Language);
8286
}
8387

8488
TEST_F(FormatTestObjC, FormatObjCTryCatch) {

0 commit comments

Comments
 (0)