amaiorano updated this revision to Diff 82814.
amaiorano added a comment.
Reverted the FallbackStyle code and added a FIXME as @ioeric suggested. I'll
fix the fallback style "none" bug in a separate change.
https://reviews.llvm.org/D28081
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Tooling/Refactoring.cpp
tools/clang-format/ClangFormat.cpp
unittests/Format/FormatTest.cpp
unittests/Format/FormatTestObjC.cpp
Index: unittests/Format/FormatTestObjC.cpp
===================================================================
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -69,15 +69,15 @@
};
TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
- Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+ Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
"- (id)init;");
EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
- Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+ Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
"+ (id)init;");
EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
// No recognizable ObjC.
- Style = getStyle("LLVM", "a.h", "none", "void f() {}");
+ Style = *getStyle("LLVM", "a.h", "none", "void f() {}");
EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
}
Index: unittests/Format/FormatTest.cpp
===================================================================
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10945,22 +10945,55 @@
ASSERT_TRUE(
FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", &FS);
- ASSERT_EQ(Style1, getLLVMStyle());
+ ASSERT_TRUE((bool)Style1);
+ ASSERT_EQ(*Style1, getLLVMStyle());
// Test 2: fallback to default.
ASSERT_TRUE(
FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", &FS);
- ASSERT_EQ(Style2, getMozillaStyle());
+ ASSERT_TRUE((bool)Style2);
+ ASSERT_EQ(*Style2, getMozillaStyle());
// Test 3: format file in parent directory.
ASSERT_TRUE(
FS.addFile("/c/.clang-format", 0,
llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
llvm::MemoryBuffer::getMemBuffer("int i;")));
auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", &FS);
- ASSERT_EQ(Style3, getGoogleStyle());
+ ASSERT_TRUE((bool)Style3);
+ ASSERT_EQ(*Style3, getGoogleStyle());
+
+ // Test 4: error on invalid fallback style
+ auto Style4 = getStyle("file", "a.h", "KungFu", "", &FS);
+ ASSERT_FALSE((bool)Style4);
+ auto ErrorMsg4 = llvm::toString(Style4.takeError());
+ ASSERT_GT(ErrorMsg4.length(), 0);
+
+ // Test 5: error on invalid yaml on command line
+ auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", &FS);
+ ASSERT_FALSE((bool)Style5);
+ auto ErrorMsg5 = llvm::toString(Style5.takeError());
+ ASSERT_GT(ErrorMsg5.length(), 0);
+
+ // Test 6: error on invalid style
+ auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", &FS);
+ ASSERT_FALSE((bool)Style6);
+ auto ErrorMsg6 = llvm::toString(Style6.takeError());
+ ASSERT_GT(ErrorMsg6.length(), 0);
+
+ // Test 7: found config file, error on parsing it
+ ASSERT_TRUE(
+ FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+ "InvalidKey: InvalidValue")));
+ ASSERT_TRUE(
+ FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+ auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS);
+ ASSERT_FALSE((bool)Style7);
+ auto ErrorMsg7 = llvm::toString(Style7.takeError());
+ ASSERT_GT(ErrorMsg7.length(), 0);
}
TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===================================================================
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,8 +249,14 @@
if (fillRanges(Code.get(), Ranges))
return true;
StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
- FormatStyle FormatStyle =
+
+ llvm::Expected<FormatStyle> FormatStyleOrError =
getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+ if (!FormatStyleOrError) {
+ llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+ return true;
+ }
+ FormatStyle FormatStyle = *FormatStyleOrError;
if (SortIncludes.getNumOccurrences() != 0)
FormatStyle.SortIncludes = SortIncludes;
unsigned CursorPosition = Cursor;
@@ -334,10 +340,16 @@
cl::PrintHelpMessage();
if (DumpConfig) {
- std::string Config =
- clang::format::configurationAsText(clang::format::getStyle(
+ llvm::Expected<clang::format::FormatStyle> FormatStyleOrError =
+ clang::format::getStyle(
Style, FileNames.empty() ? AssumeFileName : FileNames[0],
- FallbackStyle));
+ FallbackStyle);
+ if (!FormatStyleOrError) {
+ llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+ return 1;
+ }
+ std::string Config =
+ clang::format::configurationAsText(*FormatStyleOrError);
outs() << Config << "\n";
return 0;
}
Index: lib/Tooling/Refactoring.cpp
===================================================================
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -68,8 +68,8 @@
}
bool formatAndApplyAllReplacements(
- const std::map<std::string, Replacements> &FileToReplaces, Rewriter &Rewrite,
- StringRef Style) {
+ const std::map<std::string, Replacements> &FileToReplaces,
+ Rewriter &Rewrite, StringRef Style) {
SourceManager &SM = Rewrite.getSourceMgr();
FileManager &Files = SM.getFileManager();
@@ -83,7 +83,14 @@
FileID ID = SM.getOrCreateFileID(Entry, SrcMgr::C_User);
StringRef Code = SM.getBufferData(ID);
- format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+ llvm::Expected<format::FormatStyle> FormatStyleOrError =
+ format::getStyle(Style, FilePath, "LLVM");
+ if (!FormatStyleOrError) {
+ llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+ return false;
+ }
+ const format::FormatStyle CurStyle = *FormatStyleOrError;
+
auto NewReplacements =
format::formatReplacements(Code, CurReplaces, CurStyle);
if (!NewReplacements) {
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -421,6 +421,11 @@
return std::error_code(static_cast<int>(e), getParseCategory());
}
+llvm::Error make_string_error(const llvm::Twine &Message) {
+ return llvm::make_error<llvm::StringError>(Message,
+ llvm::inconvertibleErrorCode());
+}
+
const char *ParseErrorCategory::name() const noexcept {
return "clang-format.parse_error";
}
@@ -1876,9 +1881,9 @@
return FormatStyle::LK_Cpp;
}
-FormatStyle getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code,
- vfs::FileSystem *FS) {
+llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
+ StringRef FallbackStyle, StringRef Code,
+ vfs::FileSystem *FS) {
if (!FS) {
FS = vfs::getRealFileSystem().get();
}
@@ -1892,35 +1897,30 @@
(Code.contains("\n- (") || Code.contains("\n+ (")))
Style.Language = FormatStyle::LK_ObjC;
- if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style)) {
- llvm::errs() << "Invalid fallback style \"" << FallbackStyle
- << "\" using LLVM style\n";
- return Style;
- }
+ // FIXME: If FallbackStyle is explicitly "none", this effectively disables
+ // replacements. Fix this by setting a separate FormatStyle variable and
+ // returning it when we mean to return the fallback style explicitly.
+ if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style))
+ return make_string_error("Invalid fallback style \"" + FallbackStyle.str());
if (StyleName.startswith("{")) {
// Parse YAML/JSON style from the command line.
- if (std::error_code ec = parseConfiguration(StyleName, &Style)) {
- llvm::errs() << "Error parsing -style: " << ec.message() << ", using "
- << FallbackStyle << " style\n";
- }
+ if (std::error_code ec = parseConfiguration(StyleName, &Style))
+ return make_string_error("Error parsing -style: " + ec.message());
return Style;
}
if (!StyleName.equals_lower("file")) {
if (!getPredefinedStyle(StyleName, Style.Language, &Style))
- llvm::errs() << "Invalid value for -style, using " << FallbackStyle
- << " style\n";
+ return make_string_error("Invalid value for -style");
return Style;
}
// Look for .clang-format/_clang-format file in the file's parent directories.
SmallString<128> UnsuitableConfigFiles;
SmallString<128> Path(FileName);
- if (std::error_code EC = FS->makeAbsolute(Path)) {
- llvm::errs() << EC.message() << "\n";
- return Style;
- }
+ if (std::error_code EC = FS->makeAbsolute(Path))
+ return make_string_error(EC.message());
for (StringRef Directory = Path; !Directory.empty();
Directory = llvm::sys::path::parent_path(Directory)) {
@@ -1937,24 +1937,23 @@
DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
Status = FS->status(ConfigFile.str());
- bool IsFile =
+ bool FoundConfigFile =
Status && (Status->getType() == llvm::sys::fs::file_type::regular_file);
- if (!IsFile) {
+ if (!FoundConfigFile) {
// Try _clang-format too, since dotfiles are not commonly used on Windows.
ConfigFile = Directory;
llvm::sys::path::append(ConfigFile, "_clang-format");
DEBUG(llvm::dbgs() << "Trying " << ConfigFile << "...\n");
Status = FS->status(ConfigFile.str());
- IsFile = Status &&
- (Status->getType() == llvm::sys::fs::file_type::regular_file);
+ FoundConfigFile = Status && (Status->getType() ==
+ llvm::sys::fs::file_type::regular_file);
}
- if (IsFile) {
+ if (FoundConfigFile) {
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text =
FS->getBufferForFile(ConfigFile.str());
if (std::error_code EC = Text.getError()) {
- llvm::errs() << EC.message() << "\n";
- break;
+ return make_string_error(EC.message());
}
if (std::error_code ec =
parseConfiguration(Text.get()->getBuffer(), &Style)) {
@@ -1964,18 +1963,17 @@
UnsuitableConfigFiles.append(ConfigFile);
continue;
}
- llvm::errs() << "Error reading " << ConfigFile << ": " << ec.message()
- << "\n";
- break;
+ return make_string_error("Error reading " + ConfigFile + ": " +
+ ec.message());
}
DEBUG(llvm::dbgs() << "Using configuration file " << ConfigFile << "\n");
return Style;
}
}
if (!UnsuitableConfigFiles.empty()) {
- llvm::errs() << "Configuration file(s) do(es) not support "
- << getLanguageName(Style.Language) << ": "
- << UnsuitableConfigFiles << "\n";
+ return make_string_error("Configuration file(s) do(es) not support " +
+ getLanguageName(Style.Language) + ": " +
+ UnsuitableConfigFiles);
}
return Style;
}
Index: include/clang/Format/Format.h
===================================================================
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -853,17 +853,19 @@
/// \param[in] FileName Path to start search for .clang-format if ``StyleName``
/// == "file".
/// \param[in] FallbackStyle The name of a predefined style used to fallback to
-/// in case the style can't be determined from \p StyleName.
+/// in case \p StyleName is "file" and no file can be found.
/// \param[in] Code The actual code to be formatted. Used to determine the
/// language if the filename isn't sufficient.
/// \param[in] FS The underlying file system, in which the file resides. By
/// default, the file system is the real file system.
///
-/// \returns FormatStyle as specified by ``StyleName``. If no style could be
-/// determined, the default is LLVM Style (see ``getLLVMStyle()``).
-FormatStyle getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code = "",
- vfs::FileSystem *FS = nullptr);
+/// \returns FormatStyle as specified by ``StyleName``. If ``StyleName`` is
+/// "file" and no file is found, returns ``FallbackStyle``. If no style could be
+/// determined, returns an Error.
+llvm::Expected<FormatStyle> getStyle(StringRef StyleName, StringRef FileName,
+ StringRef FallbackStyle,
+ StringRef Code = "",
+ vfs::FileSystem *FS = nullptr);
// \brief Returns a string representation of ``Language``.
inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
