Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL271185: clang-format: [JS] Support shebang lines on the very 
first line. (authored by mprobst).

Changed prior to commit:
  http://reviews.llvm.org/D20632?vs=58612&id=58916#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20632

Files:
  cfe/trunk/lib/Format/FormatToken.h
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp

Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -690,10 +690,24 @@
   }
 
   LineType parsePreprocessorDirective() {
+bool IsFirstToken = CurrentToken->IsFirst;
 LineType Type = LT_PreprocessorDirective;
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript && IsFirstToken) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: cfe/trunk/lib/Format/FormatToken.h
===
--- cfe/trunk/lib/Format/FormatToken.h
+++ cfe/trunk/lib/Format/FormatToken.h
@@ -145,7 +145,7 @@
   /// \brief Whether the token text contains newlines (escaped or not).
   bool IsMultiline = false;
 
-  /// \brief Indicates that this is the first token.
+  /// \brief Indicates that this is the first token of the file.
   bool IsFirst = false;
 
   /// \brief Whether there must be a line break before this token.
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -1276,5 +1276,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -690,10 +690,24 @@
   }
 
   LineType parsePreprocessorDirective() {
+bool IsFirstToken = CurrentToken->IsFirst;
 LineType Type = LT_PreprocessorDirective;
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript && IsFirstToken) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: cfe/trunk/lib/Format/FormatToken.h
===
--- cfe/trunk/lib/Format/FormatToken.h
+++ cfe/trunk/lib/Format/FormatToken.h
@@ -145,7 +145,7 @@
   /// \brief Whether the token text contains newlines (escaped or not).
   bool IsMultiline = false;
 
-  /// \brief Indicates that this is the first token.
+  /// \brief Indicates that this is the first token of the file.
   bool IsFirst = false;
 
   /// \brief Whether there must be a line break before this token.
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -1276,5 +1276,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Daniel Jasper via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

mprobst added a comment.

Alex, do you accept this revision?


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Alex Eagle via cfe-commits]

alexeagle added a comment.

ping @djasper we need this fix in order to format angular 2 repo again


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

mprobst updated this revision to Diff 58612.
mprobst marked an inline comment as done.
mprobst added a comment.

revert FormatTokenLexer, restrict to first token


http://reviews.llvm.org/D20632

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1272,5 +1272,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -690,10 +690,24 @@
   }
 
   LineType parsePreprocessorDirective() {
+bool IsFirstToken = CurrentToken->IsFirst;
 LineType Type = LT_PreprocessorDirective;
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript && IsFirstToken) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -145,7 +145,7 @@
   /// \brief Whether the token text contains newlines (escaped or not).
   bool IsMultiline = false;
 
-  /// \brief Indicates that this is the first token.
+  /// \brief Indicates that this is the first token of the file.
   bool IsFirst = false;
 
   /// \brief Whether there must be a line break before this token.


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1272,5 +1272,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -690,10 +690,24 @@
   }
 
   LineType parsePreprocessorDirective() {
+bool IsFirstToken = CurrentToken->IsFirst;
 LineType Type = LT_PreprocessorDirective;
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript && IsFirstToken) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -145,7 +145,7 @@
   /// \brief Whether the token text contains newlines (escaped or not).
   bool IsMultiline = false;
 
-  /// \brief Indicates that this is the first token.
+  /// \brief Indicates that this is the first token of the file.
   bool IsFirst = false;
 
   /// \brief Whether there must be a line break before this token.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

mprobst marked 2 inline comments as done.


Comment at: lib/Format/TokenAnnotator.cpp:698
@@ +697,3 @@
+
+if (Style.Language == FormatStyle::LK_JavaScript) {
+  // JavaScript files can contain shebang lines of the form:

alexeagle wrote:
> should we still restrict it to be on the first line?
> 
> I suppose if you start some other line with #, that's not valid JS or TS so 
> it doesn't matter what clang-format does to it? Maybe it's fine like this.
Done. Doesn't make a big difference I guess, but it's better to be conservative.


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Alex Eagle via cfe-commits]

alexeagle added a subscriber: alexeagle.


Comment at: lib/Format/FormatTokenLexer.cpp:23
@@ -22,1 +22,3 @@
 
+#include "llvm/Support/Debug.h"
+

revert this file


Comment at: lib/Format/TokenAnnotator.cpp:698
@@ +697,3 @@
+
+if (Style.Language == FormatStyle::LK_JavaScript) {
+  // JavaScript files can contain shebang lines of the form:

should we still restrict it to be on the first line?

I suppose if you start some other line with #, that's not valid JS or TS so it 
doesn't matter what clang-format does to it? Maybe it's fine like this.


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

mprobst updated this revision to Diff 58529.
mprobst added a comment.

- use #include style shebang parsing.


http://reviews.llvm.org/D20632

Files:
  lib/Format/FormatTokenLexer.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp

Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1272,5 +1272,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -694,6 +694,19 @@
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -20,6 +20,10 @@
 #include "clang/Format/Format.h"
 #include "llvm/Support/Regex.h"
 
+#include "llvm/Support/Debug.h"
+
+#define DEBUG_TYPE "format-formatter"
+
 namespace clang {
 namespace format {
 
@@ -52,6 +56,7 @@
   tryParseTemplateString();
 }
 tryMergePreviousTokens();
+
 if (Tokens.back()->NewlinesBefore > 0 || Tokens.back()->IsMultiline)
   FirstInLineIndex = Tokens.size() - 1;
   } while (Tokens.back()->Tok.isNot(tok::eof));


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -1272,5 +1272,12 @@
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
+TEST_F(FormatTestJS, SupportShebangLines) {
+  verifyFormat("#!/usr/bin/env node\n"
+   "var x = hello();",
+   "#!/usr/bin/env node\n"
+   "var x   =  hello();");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -694,6 +694,19 @@
 next();
 if (!CurrentToken)
   return Type;
+
+if (Style.Language == FormatStyle::LK_JavaScript) {
+  // JavaScript files can contain shebang lines of the form:
+  // #!/usr/bin/env node
+  // Treat these like C++ #include directives.
+  while (CurrentToken) {
+// Tokens cannot be comments here.
+CurrentToken->Type = TT_ImplicitStringLiteral;
+next();
+  }
+  return LT_ImportStatement;
+}
+
 if (CurrentToken->Tok.is(tok::numeric_constant)) {
   CurrentToken->SpacesRequiredBefore = 1;
   return Type;
Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -20,6 +20,10 @@
 #include "clang/Format/Format.h"
 #include "llvm/Support/Regex.h"
 
+#include "llvm/Support/Debug.h"
+
+#define DEBUG_TYPE "format-formatter"
+
 namespace clang {
 namespace format {
 
@@ -52,6 +56,7 @@
   tryParseTemplateString();
 }
 tryMergePreviousTokens();
+
 if (Tokens.back()->NewlinesBefore > 0 || Tokens.back()->IsMultiline)
   FirstInLineIndex = Tokens.size() - 1;
   } while (Tokens.back()->Tok.isNot(tok::eof));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Daniel Jasper via cfe-commits]

djasper added a comment.

That's the same for #include directives (with <>). Just turn the tokens into 
TT_ImplicitStringLiteral, same as is done for #includes. I am not saying it's 
better, but I don't think we should have to different approaches..


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Martin Probst via cfe-commits]

mprobst added a comment.

How would I disable the formatting using that way?

When I special case `parsePreprocessorDirective()`, consume the line, and 
return `LT_ImportStatement`, clang-format still tries to format tokens within 
the shebang. E.g. I get `#!/usr/bin / env node`, note the whitespace after 
`bin`.

Turning the shebang into a comment seems like it has the advantage of being 
safe, its contents should simply never be touched.


http://reviews.llvm.org/D20632



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

Re: [PATCH] D20632: clang-format: [JS] Support shebang lines on the very first line.

[Daniel Jasper via cfe-commits]

djasper added a comment.

Thinking some more, I think this is actually very close to what we do for 
#include(-like) statements. That is done in TokenAnnotator::parseLine() and 
TokenAnnotator::parseIncludeDirective(). Could you move the logic there?


http://reviews.llvm.org/D20632



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