[PATCH] D115769: [clang-format] Remove spurious JSON binding when DisableFormat = true
Andrew-William-Smith created this revision.
Andrew-William-Smith added reviewers: MyDeveloperDay, HazardyKnusperkeks, jbcoe.
Andrew-William-Smith edited the summary of this revision.
Andrew-William-Smith added a project: clang-format.
Andrew-William-Smith retitled this revision from "[clang-format] Remove
spurious JSON binding when DisableFormat: true" to "[clang-format] Remove
spurious JSON binding when DisableFormat = true".
Andrew-William-Smith added a project: clang.
Andrew-William-Smith published this revision for review.
Herald added a subscriber: cfe-commits.
Relevant issue: https://github.com/llvm/llvm-project/issues/52705
When the `DisableFormat` option of `clang-format` is set to `true` and a JSON
file is formatted, the ephemeral variable binding that is added to the
top-level object is not removed from the formatted file. For example, this
JSON:
{
"key": "value"
}
Is reformatted to:
x = {
"key": "value"
}
Which is not valid JSON syntax. This fix avoids the addition of this binding
when `DisableFormat` is set to `true`, ensuring that it cannot be left behind
when formatting is disabled.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D115769
Files:
clang/tools/clang-format/ClangFormat.cpp
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -460,7 +460,7 @@
// To format JSON insert a variable to trick the code into thinking its
// JavaScript.
- if (FormatStyle->isJson()) {
+ if (FormatStyle->isJson() && !FormatStyle->DisableFormat) {
auto Err = Replaces.add(tooling::Replacement(
tooling::Replacement(AssumedFileName, 0, 0, "x = ")));
if (Err) {
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -460,7 +460,7 @@
// To format JSON insert a variable to trick the code into thinking its
// JavaScript.
- if (FormatStyle->isJson()) {
+ if (FormatStyle->isJson() && !FormatStyle->DisableFormat) {
auto Err = Replaces.add(tooling::Replacement(
tooling::Replacement(AssumedFileName, 0, 0, "x = ")));
if (Err) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D115769: [clang-format] Remove spurious JSON binding when DisableFormat = true
Andrew-William-Smith updated this revision to Diff 394582.
Andrew-William-Smith added a comment.
Added a test with formatting disabled. I had to port the updated logic from
`ClangFormat.cpp` into `FormatTestJson::format` to show the desired effect; I
also couldn't call `verifyFormat` as-is with formatting disabled since the
second assertion involving `test::messUp` would modify the formatting of the
input JSON with no way to format it back. I've added a new function
`FormatTestJson::verifyFormatStable` that only checks that formatting is stable
for this scenario.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115769/new/
https://reviews.llvm.org/D115769
Files:
clang/unittests/Format/FormatTestJson.cpp
Index: clang/unittests/Format/FormatTestJson.cpp
===
--- clang/unittests/Format/FormatTestJson.cpp
+++ clang/unittests/Format/FormatTestJson.cpp
@@ -27,7 +27,7 @@
// Mock up what ClangFormat.cpp will do for JSON by adding a variable
// to trick JSON into being JavaScript
-if (Style.isJson()) {
+if (Style.isJson() && !Style.DisableFormat) {
auto Err = Replaces.add(
tooling::Replacement(tooling::Replacement("", 0, 0, "x = ")));
if (Err) {
@@ -60,10 +60,15 @@
return Style;
}
+ static void verifyFormatStable(llvm::StringRef Code,
+ const FormatStyle &Style) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not
stable";
+ }
+
static void
verifyFormat(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Json)) {
-EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not
stable";
+verifyFormatStable(Code, Style);
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
}
};
@@ -193,5 +198,24 @@
Style);
}
+TEST_F(FormatTestJson, DisableJsonFormat) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+
+ // Since we have to disable formatting to run this test, we shall refrain
from
+ // calling test::messUp lest we change the unformatted code and cannot format
+ // it back to how it started.
+ Style.DisableFormat = true;
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+}
+
} // namespace format
} // end namespace clang
Index: clang/unittests/Format/FormatTestJson.cpp
===
--- clang/unittests/Format/FormatTestJson.cpp
+++ clang/unittests/Format/FormatTestJson.cpp
@@ -27,7 +27,7 @@
// Mock up what ClangFormat.cpp will do for JSON by adding a variable
// to trick JSON into being JavaScript
-if (Style.isJson()) {
+if (Style.isJson() && !Style.DisableFormat) {
auto Err = Replaces.add(
tooling::Replacement(tooling::Replacement("", 0, 0, "x = ")));
if (Err) {
@@ -60,10 +60,15 @@
return Style;
}
+ static void verifyFormatStable(llvm::StringRef Code,
+ const FormatStyle &Style) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+ }
+
static void
verifyFormat(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Json)) {
-EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+verifyFormatStable(Code, Style);
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
}
};
@@ -193,5 +198,24 @@
Style);
}
+TEST_F(FormatTestJson, DisableJsonFormat) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+
+ // Since we have to disable formatting to run this test, we shall refrain from
+ // calling test::messUp lest we change the unformatted code and cannot format
+ // it back to how it started.
+ Style.DisableFormat = true;
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+}
+
} // namespace format
} // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D115769: [clang-format] Remove spurious JSON binding when DisableFormat = true
Andrew-William-Smith updated this revision to Diff 394584.
Andrew-William-Smith added a comment.
Apologies, I undid the actual changes to `ClangFormat.cpp`. Still learning how
to use `arc`...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115769/new/
https://reviews.llvm.org/D115769
Files:
clang/tools/clang-format/ClangFormat.cpp
clang/unittests/Format/FormatTestJson.cpp
Index: clang/unittests/Format/FormatTestJson.cpp
===
--- clang/unittests/Format/FormatTestJson.cpp
+++ clang/unittests/Format/FormatTestJson.cpp
@@ -27,7 +27,7 @@
// Mock up what ClangFormat.cpp will do for JSON by adding a variable
// to trick JSON into being JavaScript
-if (Style.isJson()) {
+if (Style.isJson() && !Style.DisableFormat) {
auto Err = Replaces.add(
tooling::Replacement(tooling::Replacement("", 0, 0, "x = ")));
if (Err) {
@@ -60,10 +60,15 @@
return Style;
}
+ static void verifyFormatStable(llvm::StringRef Code,
+ const FormatStyle &Style) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not
stable";
+ }
+
static void
verifyFormat(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Json)) {
-EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not
stable";
+verifyFormatStable(Code, Style);
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
}
};
@@ -193,5 +198,24 @@
Style);
}
+TEST_F(FormatTestJson, DisableJsonFormat) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+
+ // Since we have to disable formatting to run this test, we shall refrain
from
+ // calling test::messUp lest we change the unformatted code and cannot format
+ // it back to how it started.
+ Style.DisableFormat = true;
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+}
+
} // namespace format
} // end namespace clang
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -460,7 +460,7 @@
// To format JSON insert a variable to trick the code into thinking its
// JavaScript.
- if (FormatStyle->isJson()) {
+ if (FormatStyle->isJson() && !FormatStyle->DisableFormat) {
auto Err = Replaces.add(tooling::Replacement(
tooling::Replacement(AssumedFileName, 0, 0, "x = ")));
if (Err) {
Index: clang/unittests/Format/FormatTestJson.cpp
===
--- clang/unittests/Format/FormatTestJson.cpp
+++ clang/unittests/Format/FormatTestJson.cpp
@@ -27,7 +27,7 @@
// Mock up what ClangFormat.cpp will do for JSON by adding a variable
// to trick JSON into being JavaScript
-if (Style.isJson()) {
+if (Style.isJson() && !Style.DisableFormat) {
auto Err = Replaces.add(
tooling::Replacement(tooling::Replacement("", 0, 0, "x = ")));
if (Err) {
@@ -60,10 +60,15 @@
return Style;
}
+ static void verifyFormatStable(llvm::StringRef Code,
+ const FormatStyle &Style) {
+EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+ }
+
static void
verifyFormat(llvm::StringRef Code,
const FormatStyle &Style = getLLVMStyle(FormatStyle::LK_Json)) {
-EXPECT_EQ(Code.str(), format(Code, Style)) << "Expected code is not stable";
+verifyFormatStable(Code, Style);
EXPECT_EQ(Code.str(), format(test::messUp(Code), Style));
}
};
@@ -193,5 +198,24 @@
Style);
}
+TEST_F(FormatTestJson, DisableJsonFormat) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_Json);
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+
+ // Since we have to disable formatting to run this test, we shall refrain from
+ // calling test::messUp lest we change the unformatted code and cannot format
+ // it back to how it started.
+ Style.DisableFormat = true;
+ verifyFormatStable("{}", Style);
+ verifyFormatStable("{\n"
+ " \"name\": 1\n"
+ "}",
+ Style);
+}
+
} // namespace format
} // end namespace clang
Index: clang/tools/clang-format/ClangFormat.cpp
===
--- clang/tools/clang-format/ClangFormat.cpp
+++ clang/tools/clang-format/ClangFormat.cpp
@@ -460,7 +460,7 @@
// To f
[PATCH] D115769: [clang-format] Remove spurious JSON binding when DisableFormat = true
Andrew-William-Smith added a comment. I'm going to assume that I don't have commit access to `main`, so that would be much appreciated. - Name: Andrew Smith - Email: aws awsmith us Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115769/new/ https://reviews.llvm.org/D115769 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
