Author: Dmitry Polukhin Date: 2020-07-09T02:41:58-07:00 New Revision: 9e7fddbd36f567217255c1df1cb816b79f0250af
URL: https://github.com/llvm/llvm-project/commit/9e7fddbd36f567217255c1df1cb816b79f0250af DIFF: https://github.com/llvm/llvm-project/commit/9e7fddbd36f567217255c1df1cb816b79f0250af.diff LOG: [yaml][clang-tidy] Fix multiline YAML serialization Summary: New line duplication logic introduced in https://reviews.llvm.org/D63482 has two issues: (1) there is no logic that removes duplicate newlines when clang-apply-replacment reads YAML and (2) in general such logic should be applied to all strings and should happen on string serialization level instead in YAML parser. This diff changes multiline strings quotation from single quote `'` to double `"`. It solves problems with internal newlines because now they are escaped. Also double quotation solves the problem with leading whitespace after newline. In case of single quotation YAML parsers should remove leading whitespace according to specification. In case of double quotation these leading are internal space and they are preserved. There is no way to instruct YAML parsers to preserve leading whitespaces after newline so double quotation is the only viable option that solves all problems at once. Test Plan: check-all Reviewers: gribozavr, mgehre, yvvan Subscribers: xazax.hun, hiraditya, cfe-commits, llvm-commits Tags: #clang-tools-extra, #clang, #llvm Differential Revision: https://reviews.llvm.org/D80301 Added: Modified: clang/include/clang/Tooling/ReplacementsYaml.h clang/unittests/Tooling/ReplacementsYamlTest.cpp llvm/include/llvm/Support/YAMLTraits.h llvm/lib/Support/YAMLTraits.cpp llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll llvm/unittests/Support/YAMLIOTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Tooling/ReplacementsYaml.h b/clang/include/clang/Tooling/ReplacementsYaml.h index 2e3e401652e2..83e35d623255 100644 --- a/clang/include/clang/Tooling/ReplacementsYaml.h +++ b/clang/include/clang/Tooling/ReplacementsYaml.h @@ -35,13 +35,7 @@ template <> struct MappingTraits<clang::tooling::Replacement> { NormalizedReplacement(const IO &, const clang::tooling::Replacement &R) : FilePath(R.getFilePath()), Offset(R.getOffset()), - Length(R.getLength()), ReplacementText(R.getReplacementText()) { - size_t lineBreakPos = ReplacementText.find('\n'); - while (lineBreakPos != std::string::npos) { - ReplacementText.replace(lineBreakPos, 1, "\n\n"); - lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2); - } - } + Length(R.getLength()), ReplacementText(R.getReplacementText()) {} clang::tooling::Replacement denormalize(const IO &) { return clang::tooling::Replacement(FilePath, Offset, Length, diff --git a/clang/unittests/Tooling/ReplacementsYamlTest.cpp b/clang/unittests/Tooling/ReplacementsYamlTest.cpp index c8fe9c4db412..3328d9bad55c 100644 --- a/clang/unittests/Tooling/ReplacementsYamlTest.cpp +++ b/clang/unittests/Tooling/ReplacementsYamlTest.cpp @@ -65,7 +65,7 @@ TEST(ReplacementsYamlTest, serializesNewLines) { " - FilePath: '/path/to/file1.h'\n" " Offset: 0\n" " Length: 0\n" - " ReplacementText: '#include <utility>\n\n'\n" + " ReplacementText: \"#include <utility>\\n\"\n" "...\n", YamlContentStream.str().c_str()); } diff --git a/llvm/include/llvm/Support/YAMLTraits.h b/llvm/include/llvm/Support/YAMLTraits.h index f93f36037679..44e34a4a09b4 100644 --- a/llvm/include/llvm/Support/YAMLTraits.h +++ b/llvm/include/llvm/Support/YAMLTraits.h @@ -649,24 +649,25 @@ inline bool isBool(StringRef S) { inline QuotingType needsQuotes(StringRef S) { if (S.empty()) return QuotingType::Single; + + QuotingType MaxQuotingNeeded = QuotingType::None; if (isSpace(static_cast<unsigned char>(S.front())) || isSpace(static_cast<unsigned char>(S.back()))) - return QuotingType::Single; + MaxQuotingNeeded = QuotingType::Single; if (isNull(S)) - return QuotingType::Single; + MaxQuotingNeeded = QuotingType::Single; if (isBool(S)) - return QuotingType::Single; + MaxQuotingNeeded = QuotingType::Single; if (isNumeric(S)) - return QuotingType::Single; + MaxQuotingNeeded = QuotingType::Single; // 7.3.3 Plain Style // Plain scalars must not begin with most indicators, as this would cause // ambiguity with other YAML constructs. static constexpr char Indicators[] = R"(-?:\,[]{}#&*!|>'"%@`)"; if (S.find_first_of(Indicators) == 0) - return QuotingType::Single; + MaxQuotingNeeded = QuotingType::Single; - QuotingType MaxQuotingNeeded = QuotingType::None; for (unsigned char C : S) { // Alphanum is safe. if (isAlnum(C)) @@ -684,11 +685,11 @@ inline QuotingType needsQuotes(StringRef S) { case 0x9: continue; // LF(0xA) and CR(0xD) may delimit values and so require at least single - // quotes. + // quotes. LLVM YAML parser cannot handle single quoted multiline so use + // double quoting to produce valid YAML. case 0xA: case 0xD: - MaxQuotingNeeded = QuotingType::Single; - continue; + return QuotingType::Double; // DEL (0x7F) are excluded from the allowed character range. case 0x7F: return QuotingType::Double; diff --git a/llvm/lib/Support/YAMLTraits.cpp b/llvm/lib/Support/YAMLTraits.cpp index 752fab2be9b3..9ac7c65e19f7 100644 --- a/llvm/lib/Support/YAMLTraits.cpp +++ b/llvm/lib/Support/YAMLTraits.cpp @@ -878,12 +878,12 @@ StringRef ScalarTraits<StringRef>::input(StringRef Scalar, void *, } void ScalarTraits<std::string>::output(const std::string &Val, void *, - raw_ostream &Out) { + raw_ostream &Out) { Out << Val; } StringRef ScalarTraits<std::string>::input(StringRef Scalar, void *, - std::string &Val) { + std::string &Val) { Val = Scalar.str(); return StringRef(); } diff --git a/llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll b/llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll index e92733f9a81a..2846889bf239 100644 --- a/llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll +++ b/llvm/test/Transforms/LowerMatrixIntrinsics/remarks-shared-subtrees.ll @@ -18,8 +18,7 @@ ; YAML-NEXT: - String: ' loads, ' ; YAML-NEXT: - NumComputeOps: '0' ; YAML-NEXT: - String: ' compute ops' -; YAML-NEXT: - String: ', -; YAML-NEXT: additionally ' +; YAML-NEXT: - String: ",\nadditionally " ; YAML-NEXT: - NumStores: '0' ; YAML-NEXT: - String: ' stores, ' ; YAML-NEXT: - NumLoads: '4' @@ -47,8 +46,7 @@ ; YAML-NEXT: - String: ' loads, ' ; YAML-NEXT: - NumComputeOps: '120' ; YAML-NEXT: - String: ' compute ops' -; YAML-NEXT: - String: ', -; YAML-NEXT: additionally ' +; YAML-NEXT: - String: ",\nadditionally " ; YAML-NEXT: - NumStores: '0' ; YAML-NEXT: - String: ' stores, ' ; YAML-NEXT: - NumLoads: '4' diff --git a/llvm/unittests/Support/YAMLIOTest.cpp b/llvm/unittests/Support/YAMLIOTest.cpp index d86489cf7560..492d854ef812 100644 --- a/llvm/unittests/Support/YAMLIOTest.cpp +++ b/llvm/unittests/Support/YAMLIOTest.cpp @@ -285,10 +285,8 @@ TEST(YAMLIO, MultilineStrings) { YOut << Original; } auto Expected = "---\n" - "str1: 'a multiline string\n" - "foobarbaz'\n" - "str2: 'another one\r" - "foobarbaz'\n" + "str1: \"a multiline string\\nfoobarbaz\"\n" + "str2: \"another one\\rfoobarbaz\"\n" "str3: a one-line string\n" "...\n"; ASSERT_EQ(Serialized, Expected); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits