Skip to content

[llvm] Avoid 'raw_string_ostream::str()' (NFC) #96995

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

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

JOE1994
Copy link
Member

@JOE1994 JOE1994 commented Jun 28, 2024

Since raw_string_ostream doesn't own the string buffer, it is desirable (in terms of memory safety) for users to directly reference the string buffer rather than use raw_string_ostream::str().

Work towards TODO comment to remove raw_string_ostream::str().

Since `raw_string_ostream` doesn't own the string buffer, it is desirable
(in terms of memory safety) for users to directly reference the string buffer
rather than use `raw_string_ostream::str()`.

Work towards TODO comment to remove `raw_string_ostream::str()`.
@llvmbot
Copy link
Member

llvmbot commented Jun 28, 2024

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-adt
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-binary-utilities

Author: Youngsuk Kim (JOE1994)

Changes

Since raw_string_ostream doesn't own the string buffer, it is desirable (in terms of memory safety) for users to directly reference the string buffer rather than use raw_string_ostream::str().

Work towards TODO comment to remove raw_string_ostream::str().


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

6 Files Affected:

  • (modified) llvm/include/llvm/ADT/FloatingPointMode.h (+1-1)
  • (modified) llvm/include/llvm/Analysis/CFGPrinter.h (+3-4)
  • (modified) llvm/include/llvm/Analysis/DDG.h (+4-4)
  • (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h (+1-1)
  • (modified) llvm/include/llvm/IR/ModuleSummaryIndex.h (+1-1)
  • (modified) llvm/include/llvm/Object/MachO.h (+2-2)
diff --git a/llvm/include/llvm/ADT/FloatingPointMode.h b/llvm/include/llvm/ADT/FloatingPointMode.h
index 468de085d1bed..639d931ef88fe 100644
--- a/llvm/include/llvm/ADT/FloatingPointMode.h
+++ b/llvm/include/llvm/ADT/FloatingPointMode.h
@@ -176,7 +176,7 @@ struct DenormalMode {
     std::string storage;
     raw_string_ostream OS(storage);
     print(OS);
-    return OS.str();
+    return storage;
   }
 };
 
diff --git a/llvm/include/llvm/Analysis/CFGPrinter.h b/llvm/include/llvm/Analysis/CFGPrinter.h
index 590b4f064b1ee..2d2a84217985a 100644
--- a/llvm/include/llvm/Analysis/CFGPrinter.h
+++ b/llvm/include/llvm/Analysis/CFGPrinter.h
@@ -133,7 +133,7 @@ std::string SimpleNodeLabelString(const BasicBlockT *Node) {
   raw_string_ostream OS(Str);
 
   Node->printAsOperand(OS, false);
-  return OS.str();
+  return Str;
 }
 
 template <typename BasicBlockT>
@@ -148,7 +148,7 @@ std::string CompleteNodeLabelString(
   std::string Str;
   raw_string_ostream OS(Str);
   HandleBasicBlock(OS, *Node);
-  std::string OutStr = OS.str();
+  std::string OutStr(Str);
   // Remove "%" from BB name
   if (OutStr[0] == '%') {
     OutStr.erase(OutStr.begin());
@@ -247,7 +247,7 @@ struct DOTGraphTraits<DOTFuncInfo *> : public DefaultDOTGraphTraits {
       raw_string_ostream OS(Str);
       auto Case = *SwitchInst::ConstCaseIt::fromSuccessorIndex(SI, SuccNo);
       OS << Case.getCaseValue()->getValue();
-      return OS.str();
+      return Str;
     }
     return "";
   }
@@ -257,7 +257,6 @@ struct DOTGraphTraits<DOTFuncInfo *> : public DefaultDOTGraphTraits {
     if (NodeName.empty()) {
       raw_string_ostream NodeOS(NodeName);
       Node->printAsOperand(NodeOS, false);
-      NodeName = NodeOS.str();
       // Removing %
       NodeName.erase(NodeName.begin());
     }
diff --git a/llvm/include/llvm/Analysis/DDG.h b/llvm/include/llvm/Analysis/DDG.h
index bd559f3fb69b1..be5f746e31a57 100644
--- a/llvm/include/llvm/Analysis/DDG.h
+++ b/llvm/include/llvm/Analysis/DDG.h
@@ -467,16 +467,16 @@ DependenceGraphInfo<NodeType>::getDependenceString(const NodeType &Src,
   raw_string_ostream OS(Str);
   DependenceList Deps;
   if (!getDependencies(Src, Dst, Deps))
-    return OS.str();
+    return Str;
   interleaveComma(Deps, OS, [&](const std::unique_ptr<Dependence> &D) {
     D->dump(OS);
     // Remove the extra new-line character printed by the dump
     // method
-    if (OS.str().back() == '\n')
-      OS.str().pop_back();
+    if (Str.back() == '\n')
+      Str.pop_back();
   });
 
-  return OS.str();
+  return Str;
 }
 
 //===--------------------------------------------------------------------===//
diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
index 67089d9e7d41b..8269344fe6efe 100644
--- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
+++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h
@@ -110,7 +110,7 @@ inline std::string hexString(uint64_t Value, size_t Width = HEX_WIDTH) {
   std::string String;
   raw_string_ostream Stream(String);
   Stream << hexValue(Value, Width, false);
-  return Stream.str();
+  return String;
 }
 
 // Get a hexadecimal string representation for the given value.
diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h
index a6bb261af7522..ecafb0e68fa7a 100644
--- a/llvm/include/llvm/IR/ModuleSummaryIndex.h
+++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h
@@ -793,7 +793,7 @@ class FunctionSummary : public GlobalValueSummary {
       OS << ", hasUnknownCall: " << this->HasUnknownCall;
       OS << ", mustBeUnreachable: " << this->MustBeUnreachable;
       OS << ")";
-      return OS.str();
+      return Output;
     }
   };
 
diff --git a/llvm/include/llvm/Object/MachO.h b/llvm/include/llvm/Object/MachO.h
index 35350df78f8d4..61d05352ec75a 100644
--- a/llvm/include/llvm/Object/MachO.h
+++ b/llvm/include/llvm/Object/MachO.h
@@ -799,7 +799,7 @@ class MachOObjectFile : public ObjectFile {
       std::string ret;
       raw_string_ostream ss(ret);
       ss << format_hex(platform, 8, true);
-      return ss.str();
+      return ret;
     }
   }
 
@@ -814,7 +814,7 @@ class MachOObjectFile : public ObjectFile {
       std::string ret;
       raw_string_ostream ss(ret);
       ss << format_hex(tools, 8, true);
-      return ss.str();
+      return ret;
     }
   }
 

@JOE1994
Copy link
Member Author

JOE1994 commented Jun 28, 2024

The following is the TODO comment I'm referring to:

/// Returns the string's reference. In most cases it is better to simply use
/// the underlying std::string directly.
/// TODO: Consider removing this API.
std::string &str() { return OS; }

@JOE1994 JOE1994 requested a review from kazutakahirata June 28, 2024 02:28
Comment on lines 148 to 150
std::string Str;
raw_string_ostream OS(Str);
HandleBasicBlock(OS, *Node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename Str to OutStr and drop the copy construction?

Suggested change
std::string Str;
raw_string_ostream OS(Str);
HandleBasicBlock(OS, *Node);
std::string OutStr;
raw_string_ostream OS(OutStr);
HandleBasicBlock(OS, *Node);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied your feedback to follow-up commit. Thank you!

@MaskRay
Copy link
Member

MaskRay commented Jun 28, 2024

LGTM once kazutakahirata's comment is addressed.

@JOE1994 JOE1994 merged commit 4a83548 into llvm:main Jun 29, 2024
7 checks passed
@JOE1994 JOE1994 deleted the move_away_from_str branch June 29, 2024 08:35
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
Since `raw_string_ostream` doesn't own the string buffer, it is
desirable (in terms of memory safety) for users to directly reference
the string buffer rather than use `raw_string_ostream::str()`.

Work towards TODO comment to remove `raw_string_ostream::str()`.
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.

4 participants