[PATCH] D156869: Remark Util intoduce remark count

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

cool!



================
Comment at: llvm/test/tools/llvm-remarkutil/annotation-count.test:15
+; COUNT-CHECK: func3,3
\ No newline at end of file

----------------
add newline


================
Comment at: llvm/test/tools/llvm-remarkutil/count/Inputs/remark-count-by.yaml:44
+  - type:            remark
\ No newline at end of file

----------------
add newline (or the YAML parser might not actually parse this)


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.cpp:92
+
+/// Look for matching argument for the key in the remark and return the parsed
+/// integer value
----------------
- if you use \p Key and \p Remark, it'll show up nice in the doxygen
- it's only the parsed integer value when you successfully find a RemarkArg, right? so the comment isn't totally accurate here IMO


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.cpp:103
+
+Error Filters::regexArgumentsValid() {
+  if (RemarkNameFilter.has_value() && RemarkNameFilter->IsRegex)
----------------
probably want doxygen comments for all functions


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.cpp:115
+  if (RemarkNameFilter.has_value())
+    if (!RemarkNameFilter->match(Remark.RemarkName))
+      return false;
----------------
can you fold these ifs?

```
if (RemarkNameFilter && !RemarkNameFilter->match(Remark.RemarkName))
  return false;
```


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:23
+
+// Collect remarks by counting the existance of a remark or by looking through
+// the keys and summing through the total count.
----------------
doxygen?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:27
+
+// Summarize the count by either emitting one count for the remark file, or
+// grouping the count by source file or by function name.
----------------
doxygen?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:36
+
+// Convert group by enums to string.
+inline std::string groupByToStr(GroupBy GroupBy) {
----------------
doxygen?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:50
+
+// Filter object which can be either a string or a regex to match with the
+// remark properties.
----------------
doxygen?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:68
+
+/// Filter out remarks based on properties.
+struct Filters {
----------------
can you expand on "properties"?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:95
+
+// Convert Regex string error to an error object.
+inline Error checkRegex(Regex &Regex) {
----------------
doxygen


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:112
+
+  /// Collect info from remark.
+  virtual void collect(const Remark &) = 0;
----------------
can you expand on "info"?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:114
+  virtual void collect(const Remark &) = 0;
+  /// Output the final count.
+  virtual Error print(StringRef OutputFileName) = 0;
----------------



================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:126
+  /// item in the row is the count for a specified key.
+  std::map<std::string, SmallVector<int, 4>> CountByKeysMap;
+  /// A set of all the keys found in the remark file. The second argument is the
----------------
- can this be a DenseMap?
- also why map strings and not the GroupBy enum?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:133
+  static Expected<KeyCounter>
+  createKeyCounter(enum GroupBy GroupBy, SmallVector<FilterMatcher, 4> &Keys,
+                   StringRef Buffer) {
----------------
comment?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:137
+    KC.GroupBy = GroupBy;
+    for (auto &Key : Keys) {
+      if (Key.IsRegex) {
----------------
can this be const?


================
Comment at: llvm/tools/llvm-remarkutil/RemarkCounter.h:168
+#endif // TOOLS_LLVM_REMARKCOUNTER_H
\ No newline at end of file

----------------
add newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156869