[PATCH] D108435: [CSSPGO] split context string II - reader/writer changes

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:754
+  /// underlying immutable buffer for all clients.
+  std::unique_ptr<std::vector<SampleContextStorageType>> CSNameTable;
+
----------------
Same as SampleProfileReaderText, can we use std::list instead of std::vector so the storage underlying will not be moved after more elements are inserted?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:239
+// `ContextStr` is in the form of `FuncName:StartLine.Discriminator`.
+void SampleProfileReaderText::decodeContextString(StringRef ContextStr,
+                                                  StringRef &FName,
----------------
Looks like it can be a utility function in SampleProfile.h and may be reused somewhere else.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:270-277
+  while (!ContextRemain.empty()) {
+    auto ContextSplit = ContextRemain.split(" @ ");
+    ChildContext = ContextSplit.first;
+    ContextRemain = ContextSplit.second;
+    LineLocation CallSiteLoc(0, 0);
+    decodeContextString(ChildContext, CalleeName, CallSiteLoc);
+    Context.emplace_back(CalleeName, CallSiteLoc);
----------------
Can we make it a utility function?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:506
+ErrorOr<SampleContext>
+SampleProfileReaderBinary::readNameFromTable(bool IsContextName) {
+  auto FName(readStringFromTable());
----------------
This param is unused. Write it as readNameFromTable(bool /* IsContextName */) to make it more explicit. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435