[PATCH] D81429: [COFF] Port CallGraphSort to COFF from ELF

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

A a few comments/suggestions.



================
Comment at: lld/Common/CallGraphSort.cpp:61
 // weights.
-CallGraphSort::CallGraphSort() {
-  MapVector<SectionPair, uint64_t> &profile = config->callGraphProfile;
-  DenseMap<const InputSectionBase *, int> secToCluster;
+template <class T, class Symbol>
+CallGraphSort<T, Symbol>::CallGraphSort(
----------------
Why `T` and not something like `Section`, similar to `Symbol`?
Perhaps I'd use `TSection` and `TSymbol` to make clear they are template names.


================
Comment at: lld/ELF/Writer.cpp:1310
+        config->callGraphProfile, config->printSymbolOrder,
+        [](const InputSectionBase *sc, Symbol *sym) {
+          if (!sym->isSection()) // Filter out section-type symbols here.
----------------
I think it is common for LLD ELF to ise `sec` for sections. So `sc` -> `sec` probably.
I'd also leave a comment to explain for what this predicate is used. Perhaps it is better not to inline it,
but define separatelly. Like 


```
 // A comment.
auto PrintSymbolOrderPred = [] (...) { ... };
...

return computeCallGraphProfileOrder<InputSectionBase, Symbol>(
        config->callGraphProfile, config->printSymbolOrder, PrintSymbolOrderPred );
```




================
Comment at: lld/include/lld/Common/CallGraphSort.h:63
+
+// Sort sections by the profile data provided by /callgraph-profile-file
+//
----------------
There is no `/callgraph-profile-file` option in ELF (it has --call-graph-ordering-file).

Since this is a common code, comments should be applicable for all platforms.


================
Comment at: lld/include/lld/Common/CallGraphSort.h:72
+    llvm::StringRef printSymbolOrder,
+    llvm::function_ref<bool(const T *, Symbol *)> cond) {
+  return CallGraphSort<T, Symbol>(profile).run(printSymbolOrder, cond);
----------------
I think it needs a comment about what is `printSymbolOrder` and what is `cond` for.

`cond` here is used as a filter predicate for `--print-symbol-order`. Its name is probably not clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81429