[PATCH] Config file support for clang-format, part 2.

Kim Gräsman kim.grasman at gmail.com
Tue May 7 14:01:31 PDT 2013


  I like where this is going, found a few nits.


================
Comment at: tools/clang-format/ClangFormat.cpp:79-80
@@ +78,4 @@
+    ConfigFile += "/.clang-format";
+    bool IsFile;
+    llvm::sys::fs::is_regular_file(ConfigFile, IsFile);
+    if (IsFile) {
----------------
Initialize IsFile or handle error return from is_regular_file. IsFile will be uninitialized if the function fails.

================
Comment at: tools/clang-format/ClangFormat.cpp:78
@@ +77,3 @@
+    std::string ConfigFile = Directory.str();
+    ConfigFile += "/.clang-format";
+    bool IsFile;
----------------
Is there a path-separator-agnostic way of doing this? "/" should be OK on Windows, but not prefered.

You could run it through llvm::sys::path::native or llvm::sys::path::append.

================
Comment at: tools/clang-format/ClangFormat.cpp:88
@@ +87,3 @@
+      FormatStyle Style;
+      if (error_code ec = parseConfiguration(Text->getBuffer(), &Style)) {
+        llvm::errs() << ec.message() << "\n";
----------------
I think that basing the YAML parser on text content will cause diagnostics not to contain the filename.

Unfortunately it looks like the smart stuff in YAMLTraits can only work based on content, not a buffer. YAMLParser's Stream class takes a MemoryBuffer and pulls filename for diagnostics from its buffer identifier, usually a filename. You may want to provoke a diagnostic by feeding the parser malformed YAML, and see what the context is. I think the default is "YAML: ".

================
Comment at: tools/clang-format/ClangFormat.cpp:92
@@ +91,3 @@
+      }
+      llvm::errs() << "Using configuration file " << ConfigFile << "\n";
+      return Style;
----------------
Remove this from stderr? Wasn't there a debug stream added to clang-format at some point?


http://llvm-reviews.chandlerc.com/D758



More information about the cfe-commits mailing list