mprobst updated this revision to Diff 240873.
mprobst marked an inline comment as done.
mprobst added a comment.
- - only run comma insertion for JavaScript.
- review fixes
- Fix col limit
- test for comma insertion
- - validate options, reject bin packing + trailing commas
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73354/new/
https://reviews.llvm.org/D73354
Files:
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestJS.cpp
Index: clang/unittests/Format/FormatTestJS.cpp
===================================================================
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -878,6 +878,45 @@
"]);");
}
+TEST_F(FormatTestJS, TrailingCommaInsertion) {
+ FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+ Style.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
+ // Insert comma in wrapped array.
+ verifyFormat("const x = [\n"
+ " 1, //\n"
+ " 2,\n"
+ "];",
+ "const x = [\n"
+ " 1, //\n"
+ " 2];",
+ Style);
+ // Insert comma in newly wrapped array.
+ Style.ColumnLimit = 30;
+ verifyFormat("const x = [\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+ "];",
+ "const x = [aaaaaaaaaaaaaaaaaaaaaaaaa];", Style);
+ // Do not insert trailing commas if they'd exceed the colum limit
+ verifyFormat("const x = [\n"
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ "];",
+ "const x = [aaaaaaaaaaaaaaaaaaaaaaaaaaaa];", Style);
+ // Object literals.
+ verifyFormat("const x = {\n"
+ " a: aaaaaaaaaaaaaaaaa,\n"
+ "};",
+ "const x = {a: aaaaaaaaaaaaaaaaa};", Style);
+ verifyFormat("const x = {\n"
+ " a: aaaaaaaaaaaaaaaaaaaaaaaaa\n"
+ "};",
+ "const x = {a: aaaaaaaaaaaaaaaaaaaaaaaaa};", Style);
+ // Object literal types.
+ verifyFormat("let x: {\n"
+ " a: aaaaaaaaaaaaaaaaaaaaa,\n"
+ "};",
+ "let x: {a: aaaaaaaaaaaaaaaaaaaaa};", Style);
+}
+
TEST_F(FormatTestJS, FunctionLiterals) {
FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13031,6 +13031,12 @@
"IndentWidth: 34",
&Style),
ParseError::Unsuitable);
+ FormatStyle BinPackedTCS = {};
+ BinPackedTCS.Language = FormatStyle::LK_JavaScript;
+ EXPECT_EQ(parseConfiguration("BinPackArguments: true\n"
+ "InsertTrailingCommas: Wrapped",
+ &BinPackedTCS),
+ ParseError::BinBackTrailingCommaConflict);
EXPECT_EQ(12u, Style.IndentWidth);
CHECK_PARSE("IndentWidth: 56", IndentWidth, 56u);
EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
Index: clang/lib/Format/Format.cpp
===================================================================
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -157,6 +157,13 @@
}
};
+template <> struct ScalarEnumerationTraits<FormatStyle::TrailingCommaStyle> {
+ static void enumeration(IO &IO, FormatStyle::TrailingCommaStyle &Value) {
+ IO.enumCase(Value, "None", FormatStyle::TCS_None);
+ IO.enumCase(Value, "Wrapped", FormatStyle::TCS_Wrapped);
+ }
+};
+
template <> struct ScalarEnumerationTraits<FormatStyle::BinaryOperatorStyle> {
static void enumeration(IO &IO, FormatStyle::BinaryOperatorStyle &Value) {
IO.enumCase(Value, "All", FormatStyle::BOS_All);
@@ -486,6 +493,7 @@
IO.mapOptional("IndentWidth", Style.IndentWidth);
IO.mapOptional("IndentWrappedFunctionNames",
Style.IndentWrappedFunctionNames);
+ IO.mapOptional("InsertTrailingCommas", Style.InsertTrailingCommas);
IO.mapOptional("JavaImportGroups", Style.JavaImportGroups);
IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
@@ -644,6 +652,8 @@
return "Invalid argument";
case ParseError::Unsuitable:
return "Unsuitable";
+ case ParseError::BinBackTrailingCommaConflict:
+ return "trailing comma insertion cannot be used with bin packing";
}
llvm_unreachable("unexpected parse error");
}
@@ -788,6 +798,7 @@
LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
LLVMStyle.IndentWrappedFunctionNames = false;
LLVMStyle.IndentWidth = 2;
+ LLVMStyle.InsertTrailingCommas = FormatStyle::TCS_None;
LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
LLVMStyle.JavaScriptWrapImports = true;
LLVMStyle.TabWidth = 8;
@@ -946,6 +957,9 @@
// taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
// commonly followed by overlong URLs.
GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
+ // TODO: enable once decided, in particular re disabling bin packing.
+ // https://google.github.io/styleguide/jsguide.html#features-arrays-trailing-comma
+ // GoogleStyle.InsertTrailingCommas = FormatStyle::TCS_Wrapped;
GoogleStyle.MaxEmptyLinesToKeep = 3;
GoogleStyle.NamespaceIndentation = FormatStyle::NI_All;
GoogleStyle.SpacesInContainerLiterals = false;
@@ -1211,6 +1225,11 @@
StyleSet.Add(std::move(DefaultStyle));
}
*Style = *StyleSet.Get(Language);
+ if (Style->InsertTrailingCommas != FormatStyle::TCS_None &&
+ Style->BinPackArguments) {
+ // See comment on FormatStyle::TSC_Wrapped.
+ return make_error_code(ParseError::BinBackTrailingCommaConflict);
+ }
return make_error_code(ParseError::Success);
}
@@ -1466,6 +1485,75 @@
FormattingAttemptStatus *Status;
};
+/// TrailingCommaInserter inserts trailing commas into container literals.
+/// E.g.:
+/// const x = [
+/// 1,
+/// ];
+/// TrailingCommaInserter runs after formatting. To avoid causing a required
+/// reformatting (and thus reflow), it never inserts a comma that'd exceed the
+/// ColumnLimit.
+///
+/// Because trailing commas disable binpacking of arrays, TrailingCommaInserter
+/// is conceptually incompatible with bin packing.
+class TrailingCommaInserter : public TokenAnalyzer {
+public:
+ TrailingCommaInserter(const Environment &Env, const FormatStyle &Style)
+ : TokenAnalyzer(Env, Style) {}
+
+ std::pair<tooling::Replacements, unsigned>
+ analyze(TokenAnnotator &Annotator,
+ SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+ FormatTokenLexer &Tokens) override {
+ AffectedRangeMgr.computeAffectedLines(AnnotatedLines);
+ tooling::Replacements Result;
+ insertTrailingCommas(AnnotatedLines, Result);
+ return {Result, 0};
+ }
+
+private:
+ /// Inserts trailing commas in [] and {} initializers if they wrap over
+ /// multiple lines.
+ void insertTrailingCommas(SmallVectorImpl<AnnotatedLine *> &Lines,
+ tooling::Replacements &Result) {
+ for (AnnotatedLine *Line : Lines) {
+ insertTrailingCommas(Line->Children, Result);
+ if (!Line->Affected)
+ continue;
+ for (FormatToken *FormatTok = Line->First; FormatTok;
+ FormatTok = FormatTok->Next) {
+ if (FormatTok->NewlinesBefore == 0)
+ continue;
+ FormatToken *Matching = FormatTok->MatchingParen;
+ if (!Matching || !FormatTok->getPreviousNonComment())
+ continue;
+ if (!(FormatTok->is(tok::r_square) &&
+ Matching->is(TT_ArrayInitializerLSquare)) &&
+ !(FormatTok->is(tok::r_brace) && Matching->is(TT_DictLiteral)))
+ continue;
+ FormatToken *Prev = FormatTok->getPreviousNonComment();
+ if (Prev->is(tok::comma) || Prev->is(tok::semi))
+ continue;
+ // getEndLoc is not reliably set during re-lexing, use text length
+ // instead.
+ SourceLocation Start =
+ Prev->Tok.getLocation().getLocWithOffset(Prev->TokenText.size());
+ // If inserting a comma would push the code over the column limit, skip
+ // this location - it'd introduce an unstable formatting due to the
+ // required reflow.
+ unsigned ColumnNumber =
+ Env.getSourceManager().getSpellingColumnNumber(Start);
+ if (ColumnNumber > Style.ColumnLimit)
+ continue;
+ // Comma insertions cannot conflict with each other, and this pass has a
+ // clean set of Replacements, so the operation below cannot fail.
+ cantFail(Result.add(
+ tooling::Replacement(Env.getSourceManager(), Start, 0, ",")));
+ }
+ }
+ }
+};
+
// This class clean up the erroneous/redundant code around the given ranges in
// file.
class Cleaner : public TokenAnalyzer {
@@ -2435,6 +2523,12 @@
return Formatter(Env, Expanded, Status).process();
});
+ if (Style.Language == FormatStyle::LK_JavaScript &&
+ Style.InsertTrailingCommas == FormatStyle::TCS_Wrapped)
+ Passes.emplace_back([&](const Environment &Env) {
+ return TrailingCommaInserter(Env, Expanded).process();
+ });
+
auto Env =
std::make_unique<Environment>(Code, FileName, Ranges, FirstStartColumn,
NextStartColumn, LastStartColumn);
Index: clang/include/clang/Format/Format.h
===================================================================
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -35,7 +35,12 @@
namespace format {
-enum class ParseError { Success = 0, Error, Unsuitable };
+enum class ParseError {
+ Success = 0,
+ Error,
+ Unsuitable,
+ BinBackTrailingCommaConflict
+};
class ParseErrorCategory final : public std::error_category {
public:
const char *name() const noexcept override;
@@ -544,6 +549,20 @@
/// \endcode
bool BinPackArguments;
+ /// The style of inserting trailing commas into container literals.
+ enum TrailingCommaStyle {
+ /// Do not insert trailing commas.
+ TCS_None,
+ /// Insert trailing commas in container literals that were wrapped over
+ /// multiple lines. Note that this is conceptually incompatible with
+ /// bin-packing, because the trailing comma is used as an indicator
+ /// that a container should be formatted one-per-line (i.e. not bin-packed).
+ /// So inserting a trailing comma counteracts bin-packing.
+ TCS_Wrapped,
+ };
+
+ TrailingCommaStyle InsertTrailingCommas;
+
/// If ``false``, a function declaration's or function definition's
/// parameters will either all be on the same line or will have one line each.
/// \code
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
