[PATCH] D141446: [llvm-profdata] Add option to cap profile output size

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.

lgtm



================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:111
+    StringBuffer.clear();
+    OutputStream.reset(new raw_svector_ostream(StringBuffer));
+    if (std::error_code EC = write(ProfileMap))
----------------
Is this leaking the raw_svector_ostream objects?

Can we rewrite this as the following?

```
raw_svector_ostream OS(StringBuffer);
OutputStream.reset(&OS);
```

Also might be worth it to use a separate OutputStream object within the loop scope so that we don't have to worry about the swap before and after and the lifetimes of the raw_vector_ostream objects.


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:71
+
+static bool operator==(const FunctionSamples &a, const FunctionSamples &b) {
+  const BodySampleMap &BodySamplesA = a.getBodySamples();
----------------
I think these parameters name should be capitalized based on the guide.
https://llvm.org/docs/CodingStandards.html

Also consider moving this to the implementation of FunctionSamples since it seems generally useful to have operator== implemented?


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:173
+    }
+    ReaderOrErr = SampleProfileReader::create(CompBinary.path().str(), Context);
+    if (!ReaderOrErr)
----------------
VAR_RETURN_IF_ERROR can be used here?


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:197
+  for (size_t OutputSizeLimit : {490, 489, 488, 475, 474, 459, 400})
+    ASSERT_THAT_EXPECTED(
+        RunTest(Input1, OutputSizeLimit, llvm::sampleprof::SPF_Ext_Binary),
----------------
EXPECT_THAT_EXPECTED is probably better to continue with other test cases rather than aborting the test on the first failure?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141446/new/

https://reviews.llvm.org/D141446