Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[formatv] Leave format parameters unstripped #112625

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

dwblaikie
Copy link
Collaborator

This is consistent with std::formatv and allows formatters to support a wider variety of use cases (like having a bare string in their formatter if that's useful, etc).

Came up in the context of some Carbon diagnostic work here: carbon-language/carbon-lang#4411 (comment)

This is consistent with std::formatv and allows formatters to support a
wider variety of use cases (like having a bare string in their formatter
if that's useful, etc).
@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-llvm-support

Author: David Blaikie (dwblaikie)

Changes

This is consistent with std::formatv and allows formatters to support a wider variety of use cases (like having a bare string in their formatter if that's useful, etc).

Came up in the context of some Carbon diagnostic work here: carbon-language/carbon-lang#4411 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/112625.diff

2 Files Affected:

  • (modified) llvm/lib/Support/FormatVariadic.cpp (+3-4)
  • (modified) llvm/unittests/Support/FormatVariadicTest.cpp (+1-1)
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index 7eb10887940513..f3e8d0a7fe6f33 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -64,11 +64,10 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   AlignStyle Where = AlignStyle::Right;
   StringRef Options;
   unsigned Index = ~0U;
-  RepString = RepString.trim();
+  RepString = RepString.ltrim();
 
   // If index is not specified, keep it ~0U to indicate unresolved index.
   RepString.consumeInteger(0, Index);
-  RepString = RepString.trim();
 
   if (RepString.consume_front(",")) {
     if (!consumeFieldLayout(RepString, Where, Align, Pad)) {
@@ -76,9 +75,9 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
       return std::nullopt;
     }
   }
-  RepString = RepString.trim();
+  RepString = RepString.ltrim();
   if (RepString.consume_front(":")) {
-    Options = RepString.trim();
+    Options = RepString;
     RepString = StringRef();
   }
   RepString = RepString.trim();
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index e745f99a5a6c4e..03102c96d46629 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -150,7 +150,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
   EXPECT_EQ(0u, Replacements[0].Index);
   EXPECT_EQ(3u, Replacements[0].Width);
   EXPECT_EQ(AlignStyle::Left, Replacements[0].Where);
-  EXPECT_EQ("foo", Replacements[0].Options);
+  EXPECT_EQ(" foo ", Replacements[0].Options);
 
   // 8. Everything after the first option specifier is part of the style, even
   // if it contains another option specifier.

@dwblaikie dwblaikie merged commit f796a0c into llvm:main Oct 16, 2024
7 of 9 checks passed
github-merge-queue bot pushed a commit to carbon-language/carbon-lang that referenced this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants