[PATCH] D68767: [clang-format] NFC - Move functionality into functions to help code structure

2019-10-12 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay abandoned this revision.
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added a comment.

Review comments regarding movign getInvalidBOM are addressed in D68914: 
[clang-format] Remove duplciate code from Invalid BOM detection 



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68767/new/

https://reviews.llvm.org/D68767



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

[PATCH] D68767: [clang-format] NFC - Move functionality into functions to help code structure

2019-10-12 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay marked 6 inline comments as done.
MyDeveloperDay added a comment.

I'll likely abandon this review when D68554: [clang-format] Proposal for 
clang-format to give compiler style warnings  
lands




Comment at: clang/tools/clang-format/ClangFormat.cpp:245
+// Returns an invalid BOM
+static const char *hasInValidBOM(StringRef BufStr) {
+  // Check to see if the buffer has a UTF Byte Order Mark (BOM).

owenpan wrote:
> owenpan wrote:
> > This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest 
> > that you move this function to there and export it through 
> > `clang/include/clang/Basic/SourceManager.h`.
> May I suggest `getInvalidBOM(const StringRef)` for the function name and 
> parameter type?
@klimek  already got me on that one over in {D68554} 



Comment at: clang/tools/clang-format/ClangFormat.cpp:245-266
+static const char *hasInValidBOM(StringRef BufStr) {
+  // Check to see if the buffer has a UTF Byte Order Mark (BOM).
+  // We only support UTF-8 with and without a BOM right now.  See
+  // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding
+  // for more information.
+  const char *InvalidBOM =
+  llvm::StringSwitch(BufStr)

MyDeveloperDay wrote:
> owenpan wrote:
> > owenpan wrote:
> > > This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest 
> > > that you move this function to there and export it through 
> > > `clang/include/clang/Basic/SourceManager.h`.
> > May I suggest `getInvalidBOM(const StringRef)` for the function name and 
> > parameter type?
> @klimek  already got me on that one over in {D68554} 
That sounds good, do you mind if I do that separately? (as this review is a 
little entwined with 
{D68554}, which is where I'll actually update that revision with your 
suggestions.



Comment at: clang/tools/clang-format/ClangFormat.cpp:381
+// Dump the configuration.
+static unsigned dumpConfig(StringRef AssumeFileName) {
+  StringRef FileName;

owenpan wrote:
> `AssumeFileName` is a file-scope global, so the file-scope function doesn't 
> need to make it a parameter? Also, a `bool` or `int` may be a better return 
> type than `unsigned`.
I think it should be `int` as it ends up being the return from main, I'll do 
that.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68767/new/

https://reviews.llvm.org/D68767



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

[PATCH] D68767: [clang-format] NFC - Move functionality into functions to help code structure

2019-10-11 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added inline comments.



Comment at: clang/tools/clang-format/ClangFormat.cpp:245
+// Returns an invalid BOM
+static const char *hasInValidBOM(StringRef BufStr) {
+  // Check to see if the buffer has a UTF Byte Order Mark (BOM).

owenpan wrote:
> This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest that 
> you move this function to there and export it through 
> `clang/include/clang/Basic/SourceManager.h`.
May I suggest `getInvalidBOM(const StringRef)` for the function name and 
parameter type?



Comment at: clang/tools/clang-format/ClangFormat.cpp:245-266
+static const char *hasInValidBOM(StringRef BufStr) {
+  // Check to see if the buffer has a UTF Byte Order Mark (BOM).
+  // We only support UTF-8 with and without a BOM right now.  See
+  // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding
+  // for more information.
+  const char *InvalidBOM =
+  llvm::StringSwitch(BufStr)

This code was copied from `clang/lib/Basic/SourceManager.cpp`. I suggest that 
you move this function to there and export it through 
`clang/include/clang/Basic/SourceManager.h`.



Comment at: clang/tools/clang-format/ClangFormat.cpp:381
+// Dump the configuration.
+static unsigned dumpConfig(StringRef AssumeFileName) {
+  StringRef FileName;

`AssumeFileName` is a file-scope global, so the file-scope function doesn't 
need to make it a parameter? Also, a `bool` or `int` may be a better return 
type than `unsigned`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68767/new/

https://reviews.llvm.org/D68767



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

[PATCH] D68767: [clang-format] NFC - Move functionality into functions to help code structure

2019-10-10 Thread MyDeveloperDay via Phabricator via cfe-commits 
MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: klimek, mitchell-stellar, owenpan.
MyDeveloperDay added a project: clang-format.
Herald added a project: clang.

I don't think its the main()'s responsibility to be working out how to dump the 
config, or the format()'s  responsibility to determine what is and isn't a 
valid BOM

move those functions out to their own functions, this gives clear roles and 
responsibilities and it makes it easier to add new additional capabilities.


Repository:
  rC Clang

https://reviews.llvm.org/D68767

Files:
  clang/tools/clang-format/ClangFormat.cpp

Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -241,6 +241,30 @@
   }
 }
 
+// Returns an invalid BOM
+static const char *hasInValidBOM(StringRef BufStr) {
+  // Check to see if the buffer has a UTF Byte Order Mark (BOM).
+  // We only support UTF-8 with and without a BOM right now.  See
+  // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding
+  // for more information.
+  const char *InvalidBOM =
+  llvm::StringSwitch(BufStr)
+  .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
+  "UTF-32 (BE)")
+  .StartsWith(llvm::StringLiteral::withInnerNUL("\xFF\xFE\x00\x00"),
+  "UTF-32 (LE)")
+  .StartsWith("\xFE\xFF", "UTF-16 (BE)")
+  .StartsWith("\xFF\xFE", "UTF-16 (LE)")
+  .StartsWith("\x2B\x2F\x76", "UTF-7")
+  .StartsWith("\xF7\x64\x4C", "UTF-1")
+  .StartsWith("\xDD\x73\x66\x73", "UTF-EBCDIC")
+  .StartsWith("\x0E\xFE\xFF", "SCSU")
+  .StartsWith("\xFB\xEE\x28", "BOCU-1")
+  .StartsWith("\x84\x31\x95\x33", "GB-18030")
+  .Default(nullptr);
+  return InvalidBOM;
+}
+
 // Returns true on error.
 static bool format(StringRef FileName) {
   if (!OutputXML && Inplace && FileName == "-") {
@@ -260,26 +284,9 @@
   if (Code->getBufferSize() == 0)
 return false; // Empty files are formatted correctly.
 
-  // Check to see if the buffer has a UTF Byte Order Mark (BOM).
-  // We only support UTF-8 with and without a BOM right now.  See
-  // https://en.wikipedia.org/wiki/Byte_order_mark#Byte_order_marks_by_encoding
-  // for more information.
   StringRef BufStr = Code->getBuffer();
-  const char *InvalidBOM =
-  llvm::StringSwitch(BufStr)
-  .StartsWith(llvm::StringLiteral::withInnerNUL("\x00\x00\xFE\xFF"),
-  "UTF-32 (BE)")
-  .StartsWith(llvm::StringLiteral::withInnerNUL("\xFF\xFE\x00\x00"),
-  "UTF-32 (LE)")
-  .StartsWith("\xFE\xFF", "UTF-16 (BE)")
-  .StartsWith("\xFF\xFE", "UTF-16 (LE)")
-  .StartsWith("\x2B\x2F\x76", "UTF-7")
-  .StartsWith("\xF7\x64\x4C", "UTF-1")
-  .StartsWith("\xDD\x73\x66\x73", "UTF-EBCDIC")
-  .StartsWith("\x0E\xFE\xFF", "SCSU")
-  .StartsWith("\xFB\xEE\x28", "BOCU-1")
-  .StartsWith("\x84\x31\x95\x33", "GB-18030")
-  .Default(nullptr);
+
+  const char *InvalidBOM = hasInValidBOM(BufStr);
 
   if (InvalidBOM) {
 errs() << "error: encoding with unsupported byte order mark \""
@@ -370,6 +377,38 @@
   OS << clang::getClangToolFullVersion("clang-format") << '\n';
 }
 
+// Dump the configuration.
+static unsigned dumpConfig(StringRef AssumeFileName) {
+  StringRef FileName;
+  std::unique_ptr Code;
+  if (FileNames.empty()) {
+// We can't read the code to detect the language if there's no
+// file name, so leave Code empty here.
+FileName = AssumeFileName;
+  } else {
+// Read in the code in case the filename alone isn't enough to
+// detect the language.
+ErrorOr> CodeOrErr =
+MemoryBuffer::getFileOrSTDIN(FileNames[0]);
+if (std::error_code EC = CodeOrErr.getError()) {
+  llvm::errs() << EC.message() << "\n";
+  return 1;
+}
+FileName = (FileNames[0] == "-") ? AssumeFileName : FileNames[0];
+Code = std::move(CodeOrErr.get());
+  }
+  llvm::Expected FormatStyle =
+  clang::format::getStyle(Style, FileName, FallbackStyle,
+  Code ? Code->getBuffer() : "");
+  if (!FormatStyle) {
+llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
+return 1;
+  }
+  std::string Config = clang::format::configurationAsText(*FormatStyle);
+  outs() << Config << "\n";
+  return 0;
+}
+
 int main(int argc, const char **argv) {
   llvm::InitLLVM X(argc, argv);
 
@@ -391,34 +430,7 @@
   }
 
   if (DumpConfig) {
-StringRef FileName;
-std::unique_ptr Code;
-if (FileNames.empty()) {
-  // We can't read the code to detect the language if there's no
-  // file name, so leave Code empty here.
-  FileName = AssumeFileName;
-} else {
-  // Read in the code in case the filename alone isn't enough to
-  // detect the