Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-20 Thread Martin Probst via cfe-commits 
This revision was automatically updated to reflect the committed changes.
mprobst marked 2 inline comments as done.
Closed by commit rL270203: clang-format: [JS] sort ES6 imports. (authored by 
mprobst).

Changed prior to commit:
  http://reviews.llvm.org/D20198?vs=57795=57922#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20198

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/CMakeLists.txt
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/FormatToken.h
  cfe/trunk/lib/Format/FormatTokenLexer.cpp
  cfe/trunk/lib/Format/FormatTokenLexer.h
  cfe/trunk/lib/Format/SortJavaScriptImports.cpp
  cfe/trunk/lib/Format/SortJavaScriptImports.h
  cfe/trunk/lib/Format/TokenAnalyzer.cpp
  cfe/trunk/lib/Format/TokenAnalyzer.h
  cfe/trunk/unittests/Format/CMakeLists.txt
  cfe/trunk/unittests/Format/SortImportsTestJS.cpp

Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -852,6 +852,22 @@
 FormatStyle getStyle(StringRef StyleName, StringRef FileName,
  StringRef FallbackStyle, vfs::FileSystem *FS = nullptr);
 
+// \brief Returns a string representation of ``Language``.
+inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
+  switch (Language) {
+  case FormatStyle::LK_Cpp:
+return "C++";
+  case FormatStyle::LK_Java:
+return "Java";
+  case FormatStyle::LK_JavaScript:
+return "JavaScript";
+  case FormatStyle::LK_Proto:
+return "Proto";
+  default:
+return "Unknown";
+  }
+}
+
 } // end namespace format
 } // end namespace clang
 
Index: cfe/trunk/lib/Format/SortJavaScriptImports.h
===
--- cfe/trunk/lib/Format/SortJavaScriptImports.h
+++ cfe/trunk/lib/Format/SortJavaScriptImports.h
@@ -0,0 +1,36 @@
+//===--- SortJavaScriptImports.h - Sort ES6 Imports -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+/// \file
+/// \brief This file implements a sorter for JavaScript ES6 imports.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_FORMAT_SORTJAVASCRIPTIMPORTS_H
+#define LLVM_CLANG_LIB_FORMAT_SORTJAVASCRIPTIMPORTS_H
+
+#include "clang/Basic/LLVM.h"
+#include "clang/Format/Format.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace format {
+
+// Sort JavaScript ES6 imports/exports in ``Code``. The generated replacements
+// only monotonically increase the length of the given code.
+tooling::Replacements sortJavaScriptImports(const FormatStyle ,
+StringRef Code,
+ArrayRef Ranges,
+StringRef FileName);
+
+} // end namespace format
+} // end namespace clang
+
+#endif
Index: cfe/trunk/lib/Format/FormatToken.h
===
--- cfe/trunk/lib/Format/FormatToken.h
+++ cfe/trunk/lib/Format/FormatToken.h
@@ -535,6 +535,7 @@
 kw_NS_ENUM = ("NS_ENUM");
 kw_NS_OPTIONS = ("NS_OPTIONS");
 
+kw_as = ("as");
 kw_async = ("async");
 kw_await = ("await");
 kw_finally = ("finally");
@@ -585,6 +586,7 @@
   IdentifierInfo *kw___except;
 
   // JavaScript keywords.
+  IdentifierInfo *kw_as;
   IdentifierInfo *kw_async;
   IdentifierInfo *kw_await;
   IdentifierInfo *kw_finally;
Index: cfe/trunk/lib/Format/FormatTokenLexer.h
===
--- cfe/trunk/lib/Format/FormatTokenLexer.h
+++ cfe/trunk/lib/Format/FormatTokenLexer.h
@@ -0,0 +1,97 @@
+//===--- FormatTokenLexer.h - Format C++ code *- C++ *-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+/// \file
+/// \brief This file contains FormatTokenLexer, which tokenizes a source file
+/// into a token stream suitable for ClangFormat.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_FORMAT_FORMATTOKENLEXER_H
+#define LLVM_CLANG_LIB_FORMAT_FORMATTOKENLEXER_H
+
+#include "Encoding.h"
+#include "FormatToken.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Regex.h"
+
+namespace clang {
+namespace format {
+
+class FormatTokenLexer {
+public:
+  FormatTokenLexer(const SourceManager ,

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-19 Thread Martin Probst via cfe-commits 
mprobst marked 2 inline comments as done.
mprobst added a comment.

Thanks for the review, appreciated!



Comment at: lib/Format/SortJavaScriptImports.cpp:160
@@ +159,3 @@
+  if (i + 1 < e) {
+// Insert breaks between imports.
+ReferencesText += "\n";

klimek wrote:
> Between categories of imports and imports and exports, right?
Added a comment.


Comment at: lib/Format/SortJavaScriptImports.cpp:170-171
@@ +169,4 @@
+// Separate references from the main code body of the file.
+if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2)
+  ReferencesText += "\n";
+

klimek wrote:
> Shouldn't we add 2 if NewlinesBefore is 0? Or is that syntactically 
> impossible?
The next token is on a new line, so it gets a wrap just from that already. I've 
added a test though.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-19 Thread Manuel Klimek via cfe-commits 
klimek added inline comments.


Comment at: lib/Format/SortJavaScriptImports.cpp:160
@@ +159,3 @@
+  if (i + 1 < e) {
+// Insert breaks between imports.
+ReferencesText += "\n";

Between categories of imports and imports and exports, right?


Comment at: lib/Format/SortJavaScriptImports.cpp:170-171
@@ +169,4 @@
+// Separate references from the main code body of the file.
+if (FirstNonImportLine && FirstNonImportLine->First->NewlinesBefore < 2)
+  ReferencesText += "\n";
+

Shouldn't we add 2 if NewlinesBefore is 0? Or is that syntactically impossible?


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-19 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57795.
mprobst added a comment.

- ranges


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,194 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::string sort(StringRef Code, unsigned Offset = 0, unsigned Length = 0) {
+StringRef FileName = "input.js";
+if (Length == 0U)
+  Length = Code.size() - Offset;
+std::vector Ranges(1, tooling::Range(Offset, Length));
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code,
+  unsigned Offset = 0, unsigned Length = 0) {
+std::string Result = sort(Code, Offset, Length);
+EXPECT_EQ(Expected.str(), Result) << "Expected:\n"
+  << Expected << "\nActual:\n"
+  << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, AlreadySorted) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-19 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57794.
mprobst added a comment.

- correctly insert breaks after import block


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,180 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::string sort(StringRef Code, unsigned Offset = 0, unsigned Length = 0) {
+StringRef FileName = "input.js";
+if (Length == 0U)
+  Length = Code.size() - Offset;
+std::vector Ranges(1, tooling::Range(Offset, Length));
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code,
+  unsigned Offset = 0, unsigned Length = 0) {
+std::string Result = sort(Code, Offset, Length);
+EXPECT_EQ(Expected.str(), Result) << "Expected:\n"
+  << Expected << "\nActual:\n"
+  << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, AlreadySorted) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-19 Thread Manuel Klimek via cfe-commits 
klimek accepted this revision.
klimek added a comment.

lg


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57637.
mprobst marked an inline comment as done.
mprobst added a comment.

- address review comments
- - extract parseModuleReferences
- more review comments


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Martin Probst via cfe-commits 
mprobst marked 4 inline comments as done.


Comment at: lib/Format/SortJavaScriptImports.cpp:216-217
@@ +215,4 @@
+break;
+  Current = Line->First;
+  LineEnd = Line->Last;
+  skipComments();

klimek wrote:
> Both of these are used only once, perhaps inline?
These are setting fields for the parse state (note: this is not a variable 
declaration).


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Manuel Klimek via cfe-commits 
klimek added a comment.

We're getting there. Couple of nits left.



Comment at: lib/Format/SortJavaScriptImports.cpp:94-97
@@ +93,6 @@
+// Side effect imports might be ordering sensitive. Consider them equal so
+// that they maintain their relative order in the stable sort below.
+// This retains transitivity because LHS.Category == RHS.Category here.
+return false;
+  // Empty URLs sort *last* (for export {...};).
+  if (LHS.URL.empty() != RHS.URL.empty())

Yea, completely missed that the != above.


Comment at: lib/Format/SortJavaScriptImports.cpp:128
@@ +127,3 @@
+SmallVector References;
+parseModuleReferences(Keywords, AnnotatedLines, References);
+

Return by value.


Comment at: lib/Format/SortJavaScriptImports.cpp:216-217
@@ +215,4 @@
+break;
+  Current = Line->First;
+  LineEnd = Line->Last;
+  skipComments();

Both of these are used only once, perhaps inline?


Comment at: lib/Format/SortJavaScriptImports.cpp:229
@@ +228,3 @@
+  Reference.Range.setBegin(Start);
+  Start = SourceLocation();
+  if (!parseModuleReference(Keywords, Reference))

I'd put that down after References.push_back so calculating the Reference is at 
least a single flow.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Martin Probst via cfe-commits 
mprobst added a comment.

PTAL.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57621.
mprobst marked 2 inline comments as done.
mprobst added a comment.

- address review comments
- - extract parseModuleReferences


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from '../parent';\n"
+ "let

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Martin Probst via cfe-commits 
mprobst marked 6 inline comments as done.
mprobst added a comment.

PTAL.



Comment at: lib/Format/SortJavaScriptImports.cpp:93-96
@@ +92,6 @@
+return LHS.Category < RHS.Category;
+  if (LHS.Category == JsModuleReference::ReferenceCategory::SIDE_EFFECT)
+// Side effect imports might be ordering sensitive. Consider them equal so
+// that they maintain their relative order in the stable sort below.
+return false;
+  // Empty URLs sort *last* (for export {...};).

klimek wrote:
> Doesn't that break strict weak ordering requirements?
> 
> x = non-side-effect (url: a)
> y = side-effect (url: b)
> z = non-side-effect (url: c)
> now x and y are incomparable:
> !(x < y) // because x.url < y.url
> && !(y < x) // because of y == side-effect
> but x and z are comparable:
> x < z
> breaks transitivity of incomparability 
> (https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings)
> 
> To keep them equal, wouldn't you need:
>   if (LHS == SIDE_EFFECT && RHS == SIDE_EFFECT)
> return false;
> (that would at least seem symmetric)
As discussed offline, the comparison before makes sure that RHS == SIDE_EFFECT 
here.

Added a comment.


Comment at: lib/Format/SortJavaScriptImports.cpp:135
@@ +134,3 @@
+  LineEnd = Line->Last;
+  JsModuleReference Reference;
+  skipComments();

klimek wrote:
> Why are you defining this here, far before its use?
> 
> I think this code might be slightly simpler (for me to understand) if you 
> just pulled Reference out of the loop, and got rid of LastStart.
> 
> 
As discussed offline, moved the declaration a bit down.

I think exposing just a `SourceLocation Start` makes the algorithm easier to 
read, otherwise it'd look like a whole `JsModuleReference` could leave the 
loop, which is never the case. It explicitly points out we only have state 
outside of the loop to track the start.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-18 Thread Manuel Klimek via cfe-commits 
klimek added inline comments.


Comment at: lib/Format/SortJavaScriptImports.cpp:46-47
@@ +45,4 @@
+// An ES6 module reference.
+//
+// ES6 implements a module system, where individual modules (~= source files)
+// can reference other modules, either importing symbols from them, or 
exporting

Thanks, this is much better.


Comment at: lib/Format/SortJavaScriptImports.cpp:84-85
@@ +83,4 @@
+  // comments.
+  SourceLocation Start;
+  SourceLocation End;
+};

Any reason you're not using a SourceRange?


Comment at: lib/Format/SortJavaScriptImports.cpp:93-96
@@ +92,6 @@
+return LHS.Category < RHS.Category;
+  if (LHS.Category == JsModuleReference::ReferenceCategory::SIDE_EFFECT)
+// Side effect imports might be ordering sensitive. Consider them equal so
+// that they maintain their relative order in the stable sort below.
+return false;
+  // Empty URLs sort *last* (for export {...};).

Doesn't that break strict weak ordering requirements?

x = non-side-effect (url: a)
y = side-effect (url: b)
z = non-side-effect (url: c)
now x and y are incomparable:
!(x < y) // because x.url < y.url
&& !(y < x) // because of y == side-effect
but x and z are comparable:
x < z
breaks transitivity of incomparability 
(https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings)

To keep them equal, wouldn't you need:
  if (LHS == SIDE_EFFECT && RHS == SIDE_EFFECT)
return false;
(that would at least seem symmetric)


Comment at: lib/Format/SortJavaScriptImports.cpp:128
@@ +127,3 @@
+
+SmallVector Imports;
+SourceLocation LastStart;

It seems like you can pull the loop below into its own function 
parseModuleReferences or something.


Comment at: lib/Format/SortJavaScriptImports.cpp:129
@@ +128,3 @@
+SmallVector Imports;
+SourceLocation LastStart;
+for (auto Line : AnnotatedLines) {

LastStart is just the start of the currently processed module reference, right? 
I'd just call it Start then.


Comment at: lib/Format/SortJavaScriptImports.cpp:135
@@ +134,3 @@
+  LineEnd = Line->Last;
+  JsModuleReference Reference;
+  skipComments();

Why are you defining this here, far before its use?

I think this code might be slightly simpler (for me to understand) if you just 
pulled Reference out of the loop, and got rid of LastStart.




http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-17 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57497.
mprobst marked 8 inline comments as done.
mprobst added a comment.

- address review comments


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from '../parent';\n"
+ "let x = 1;\n"
+ "export y

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-17 Thread Manuel Klimek via cfe-commits 
klimek added inline comments.


Comment at: include/clang/Format/Format.h:855
@@ -854,1 +854,3 @@
 
+// \brief Returns a string representation of ``Language`` for debugging.
+inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {

s/for debugging/.
Or do you want to claim that we'll arbitrarily change the strings here?


Comment at: lib/Format/Format.cpp:1217-1224
@@ -1983,8 +1216,10 @@
 
 tooling::Replacements sortIncludes(const FormatStyle , StringRef Code,
ArrayRef Ranges,
StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
   if (!Style.SortIncludes)
 return Replaces;
+  if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript)
+return sortJavaScriptImports(Style, Code, Ranges, FileName);
 

I'd make this symmetric by pulling out 2 functions instead of falling through 
into the function.


Comment at: lib/Format/SortJavaScriptImports.cpp:45
@@ +44,3 @@
+
+struct JsImportExport {
+  bool IsExport;

klimek wrote:
> This is really weird - a class called ImportExport that then has a bool 
> IsExport. Please explain in a comment why you're doing that instead of any of:
> a) managing a set of imports and exports on their own
> b) calling this something else
> c) having a class hierarchy
As not all of the members will be default initialized I'd prefer to have 
constructors.


Comment at: lib/Format/SortJavaScriptImports.cpp:45-46
@@ +44,4 @@
+
+struct JsImportExport {
+  bool IsExport;
+  // JS imports are sorted into these categories, in order.

This is really weird - a class called ImportExport that then has a bool 
IsExport. Please explain in a comment why you're doing that instead of any of:
a) managing a set of imports and exports on their own
b) calling this something else
c) having a class hierarchy


Comment at: lib/Format/SortJavaScriptImports.cpp:115
@@ +114,3 @@
+  LineEnd = Line->Last;
+  JsImportExport ImpExp;
+  skipComments();

When I see ImpExp I think "ImplicitExpression". I'd name this ImportExport.
Also, it seems like we now have an uninitialized bool and enum in the struct.


Comment at: lib/Format/SortJavaScriptImports.cpp:163
@@ +162,3 @@
+  return Result;
+
+// Replace all existing import/export statements.

If that's the case, please add a comment to that end.


Comment at: lib/Format/SortJavaScriptImports.cpp:207
@@ +206,3 @@
+
+  StringRef getSourceText(SourceLocation Start, SourceLocation End) {
+const SourceManager  = Env.getSourceManager();

Usually we call Lexer::getSourceText for that (don't we have a Lexer?)


Comment at: lib/Format/SortJavaScriptImports.cpp:252-253
@@ +251,4 @@
+
+  bool parseImportExportSpecifier(const AdditionalKeywords ,
+  JsImportExport ) {
+// * as prefix from '...';

This function is really long - can we perhaps break out smaller syntactic 
options.


Comment at: lib/Format/SortJavaScriptImports.cpp:258
@@ +257,3 @@
+return false;
+  if (!Current->is(Keywords.kw_as) || !nextToken())
+return false;

Please don't put side-effects into an if (). This might be generally easier to 
read if nextToken() can't fail, and instead creates an invalid token.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-17 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57440.
mprobst marked an inline comment as done.
mprobst added a comment.

- review comments


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from '../parent';\n"
+ "let x = 1;\n"
+ "export y =

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-17 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57438.
mprobst marked an inline comment as done.
mprobst added a comment.

- extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
- clean up imports/
- includes
- address review comments
- pull out implementations from header files.
- support side effect imports, keep in relative order at top.
- Move getLanguageName to Format.h


http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-17 Thread Daniel Jasper via cfe-commits 
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

I think, this looks good.

Manuel: Are you fine with this, design-wise?



Comment at: lib/Format/SortJavaScriptImports.cpp:77
@@ +76,3 @@
+return false;
+  // NB: empty URLs sort *last* (for export {...};).
+  if (LHS.URL.empty() != RHS.URL.empty())

NB?


Comment at: lib/Format/SortJavaScriptImports.cpp:255
@@ +254,3 @@
+// * as prefix from '...';
+if (Current->is(tok::star)) {
+  if (!nextToken())

We probably should really have a variant of AnnotatedLine::startsWith() that 
can start at an arbitrary token. But we can try that in a follow-up.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-13 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57138.
mprobst added a comment.

- extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
- clean up imports/
- includes
- address review comments
- pull out implementations from header files.
- support side effect imports, keep in relative order at top.


http://reviews.llvm.org/D20198

Files:
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,137 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst added a comment.

PTAL.



Comment at: lib/Format/SortJavaScriptImports.cpp:162
@@ +161,3 @@
+std::string ImportsText;
+for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
+  JsImportExport ImpExp = Imports[Indices[i]];

djasper wrote:
> Is there any chance you are changing the total length of an import block?
No, this only ever monotonously increases the length. I'm aware things fall 
apart if it doesn't ;-)


Comment at: lib/Format/SortJavaScriptImports.cpp:226
@@ +225,3 @@
+  ImpExp.URL = Current->TokenText.substr(1, Current->TokenText.size() - 2);
+  if (ImpExp.URL.startswith("..")) {
+ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE_PARENT;

djasper wrote:
> No braces.
Just can't get used to that, it feels so wrong...


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57073.
mprobst marked 9 inline comments as done.
mprobst added a comment.

- extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
- clean up imports/
- includes
- address review comments
- pull out implementations from header files.


http://reviews.llvm.org/D20198

Files:
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.cpp
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.cpp
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Daniel Jasper via cfe-commits 
djasper added inline comments.


Comment at: lib/Format/Format.cpp:21
@@ -19,1 +20,3 @@
 #include "TokenAnnotator.h"
+#include "FormatTokenLexer.h"
+#include "TokenAnalyzer.h"

Use clang-format to fix the order :-)


Comment at: lib/Format/FormatTokenLexer.h:28
@@ +27,3 @@
+
+class FormatTokenLexer {
+public:

I believe it is a bit harmful to have that much code in a header. It now gets 
pulled into multiple translation units and is thus compiled multiple times only 
for the linker to deduplicate it afterwards. We should move the implementation 
in FormatTokenLexer.cpp. I am happy to do that as a follow up, though.


Comment at: lib/Format/SortJavaScriptImports.cpp:54
@@ +53,3 @@
+  JsImportCategory Category;
+  // Empty for `export {a, b};`.
+  StringRef URL;

This comment doesn't really explain what it is and it is not obvious to me.


Comment at: lib/Format/SortJavaScriptImports.cpp:112
@@ +111,3 @@
+  skipComments();
+  if (LastStart.isInvalid() || Imports.empty()) {
+// After the first file level comment, consider line comments to be 
part

No braces.


Comment at: lib/Format/SortJavaScriptImports.cpp:150
@@ +149,3 @@
+
+bool OutOfOrder = false;
+for (unsigned i = 0, e = Indices.size(); i != e; ++i) {

Add:

  // FIXME: Pull this into a common function.


Comment at: lib/Format/SortJavaScriptImports.cpp:162
@@ +161,3 @@
+std::string ImportsText;
+for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
+  JsImportExport ImpExp = Imports[Indices[i]];

Is there any chance you are changing the total length of an import block?


Comment at: lib/Format/SortJavaScriptImports.cpp:226
@@ +225,3 @@
+  ImpExp.URL = Current->TokenText.substr(1, Current->TokenText.size() - 2);
+  if (ImpExp.URL.startswith("..")) {
+ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE_PARENT;

No braces.


Comment at: lib/Format/SortJavaScriptImports.cpp:291
@@ +290,3 @@
+unsigned *Cursor) {
+  // TODO(martinprobst): Cursor support.
+  std::unique_ptr Env =

s/TODO(martinprobst)/FIXME/

Also, I'd probably remove the parameter until this is supported. This FIXME is 
quite hidden but if there is no Cursor in the interface, there is no confusion.


Comment at: lib/Format/TokenAnalyzer.h:1
@@ +1,2 @@
+//===--- TokenAnalyzer.h - Analyze Token Streams *- C++ 
-*-===//
+//

This isn't quite as bad as the other, but we probably want a dedicated .cpp 
file, too.


http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57039.
mprobst added a comment.

- extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
- clean up imports/
- includes


http://reviews.llvm.org/D20198

Files:
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from '../parent';\n"
+ "let x = 1;\n"
+ "export y = 1;\n");
+}
+
+} // end namespace
+} // end namespace format
+} // end

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst marked 2 inline comments as done.
mprobst added a comment.

http://reviews.llvm.org/D20198



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

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst updated this revision to Diff 57037.
mprobst added a comment.

- extract TokenAnalyzer.h and SortJavaScriptImports.h/cpp
- clean up imports


http://reviews.llvm.org/D20198

Files:
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/FormatTokenLexer.h
  lib/Format/SortJavaScriptImports.cpp
  lib/Format/SortJavaScriptImports.h
  lib/Format/TokenAnalyzer.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted =
+applyAllReplacements(Code, sortIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+ "export {A, B} from 'apath';\n"
+ "import {S} from 'bpath';\n"
+ "export {P} from '../parent';\n"
+ "let x = 1;\n"
+ "export y = 1;\n");
+}
+
+} // end namespace
+} // end namespace format
+} // end namespace clang

Re: [PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Daniel Jasper via cfe-commits 
djasper added a comment.

Just two high-level comments. Will review in more depth later.



Comment at: include/clang/Format/Format.h:770
@@ +769,3 @@
+/// ``export`` blocks are affected by ``Ranges``.
+tooling::Replacements sortJavaScriptIncludes(const FormatStyle ,
+ StringRef Code,

Why is this a separate function as opposed to looking at Style.Language?


Comment at: lib/Format/Format.cpp:1958
@@ +1957,3 @@
+// An imported symbol in a JavaScript ES6 import/export, possibly aliased.
+struct JsImportedSymbol {
+  StringRef Symbol;

I know this file is already a huge mess, but can you move all of the JS import 
sorting stuff to a separate file?


http://reviews.llvm.org/D20198



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

[PATCH] D20198: clang-format: [JS] sort ES6 imports.

2016-05-12 Thread Martin Probst via cfe-commits 
mprobst created this revision.
mprobst added a reviewer: djasper.
mprobst added a subscriber: cfe-commits.
Herald added a subscriber: klimek.

This change automatically sorts ES6 imports and exports into four groups:
absolute imports, parent imports, relative imports, and then exports. Exports
are sorted in the same order, but not grouped further.

http://reviews.llvm.org/D20198

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  unittests/Format/CMakeLists.txt
  unittests/Format/SortImportsTestJS.cpp

Index: unittests/Format/SortImportsTestJS.cpp
===
--- /dev/null
+++ unittests/Format/SortImportsTestJS.cpp
@@ -0,0 +1,130 @@
+//===- unittest/Format/SortImportsTestJS.cpp - JS import sort unit tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FormatTestUtils.h"
+#include "clang/Format/Format.h"
+#include "llvm/Support/Debug.h"
+#include "gtest/gtest.h"
+
+#define DEBUG_TYPE "format-test"
+
+namespace clang {
+namespace format {
+namespace {
+
+class SortImportsTestJS : public ::testing::Test {
+protected:
+  std::vector GetCodeRange(StringRef Code) {
+return std::vector(1, tooling::Range(0, Code.size()));
+  }
+
+  std::string sort(StringRef Code, StringRef FileName = "input.js") {
+auto Ranges = GetCodeRange(Code);
+std::string Sorted = applyAllReplacements(
+Code, sortJavaScriptIncludes(Style, Code, Ranges, FileName));
+return applyAllReplacements(Sorted,
+reformat(Style, Sorted, Ranges, FileName));
+  }
+
+  void verifySort(llvm::StringRef Expected, llvm::StringRef Code) {
+std::string Result = sort(Code);
+EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  unsigned newCursor(llvm::StringRef Code, unsigned Cursor) {
+sortJavaScriptIncludes(Style, Code, GetCodeRange(Code), "input.js",
+   );
+return Cursor;
+  }
+
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+};
+
+TEST_F(SortImportsTestJS, BasicSorting) {
+  verifySort("import {sym} from 'a';\n"
+ "import {sym} from 'b';\n"
+ "import {sym} from 'c';\n"
+ "\n"
+ "let x = 1;",
+ "import {sym} from 'a';\n"
+ "import {sym} from 'c';\n"
+ "import {sym} from 'b';\n"
+ "let x = 1;");
+}
+
+TEST_F(SortImportsTestJS, Comments) {
+  verifySort("/** @fileoverview This is a great file. */\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n"
+ "import {sym} from 'b'; // from //foo:bar\n",
+ "/** @fileoverview This is a great file. */\n"
+ "import {sym} from 'b'; // from //foo:bar\n"
+ "// A very important import follows.\n"
+ "import {sym} from 'a'; /* more comments */\n");
+}
+
+TEST_F(SortImportsTestJS, SortStar) {
+  verifySort("import * as foo from 'a';\n"
+ "import {sym} from 'a';\n"
+ "import * as bar from 'b';\n",
+ "import {sym} from 'a';\n"
+ "import * as foo from 'a';\n"
+ "import * as bar from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, AliasesSymbols) {
+  verifySort("import {sym1 as alias1} from 'b';\n"
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n",
+ "import {sym2 as alias2, sym3 as alias3} from 'c';\n"
+ "import {sym1 as alias1} from 'b';\n");
+}
+
+TEST_F(SortImportsTestJS, GroupImports) {
+  verifySort("import {a} from 'absolute';\n"
+ "\n"
+ "import {b} from '../parent';\n"
+ "import {b} from '../parent/nested';\n"
+ "\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from './relative/path/nested';\n"
+ "\n"
+ "let x = 1;\n",
+ "import {b} from './relative/path/nested';\n"
+ "import {b} from './relative/path';\n"
+ "import {b} from '../parent/nested';\n"
+ "import {b} from '../parent';\n"
+ "import {a} from 'absolute';\n"
+ "let x = 1;\n");
+}
+
+TEST_F(SortImportsTestJS, Exports) {
+  verifySort("import {S} from 'bpath';\n"
+ "\n"
+ "import {T} from './cpath';\n"
+ "\n"
+ "export {A, B} from 'apath';\n"
+ "export {P} from '../parent';\n"
+ "export {R} from './relative';\n"
+ "export {S};\n"
+ "\n"
+ "let x = 1;\n"
+ "export y = 1;\n",
+ "export {R} from './relative';\n"
+ "import {T} from './cpath';\n"
+ "export {S};\n"
+