[PATCH] D141446: Fix to D139603(reverted) - moved size check to unit test so that it is cross-platform

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm with some comments.



================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:100
+
+  size_t OriginalFunctionCount = ProfileMap.size();
+
----------------
This will become unused in non-DEBUG build, maybe add the workaround here https://reviews.llvm.org/rG9f4a9d3f44501fa755eb71fe855e15cf0e59e8b8

Or wrap this in LLVM_DEBUG too.

Also IterationCount below as mentioned in this comment on the previous revision: https://reviews.llvm.org/D139603#inline-1365026


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:41
+  if (std::error_code EC = ReaderOrErr.getError())
+    report_fatal_error(EC.message().c_str());
+  auto Reader = std::move(ReaderOrErr.get());
----------------
I think we should replace report_fatal_error with 

```
std::error_code EC = ReaderOrErr.getError();
ASSERT_FALSE(EC) << EC.message().c_str();
```
to let gtest handle the failure cleanly.


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:69
+
+TEST(TestOutputSizeLimit, TestOutputSizeLimit1) {
+  for (size_t OutputSizeLimit : {489, 488, 474})
----------------
Perhaps TestWriteWithSizeLimit instead of TestOutputSizeLimit1 is a little more informative?


================
Comment at: llvm/unittests/tools/llvm-profdata/OutputSizeLimitTest.cpp:71
+  for (size_t OutputSizeLimit : {489, 488, 474})
+    ASSERT_LE(WriteProfile(Input1, OutputSizeLimit), OutputSizeLimit);
+}
----------------
This should probably be EXPECT_LE.
The rationale is explained here https://stackoverflow.com/a/2565309


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141446