[PATCH] D58675: [clang] Adds `-ftime-trace` option to clang that produces Chrome `chrome://tracing` compatible JSON profiling output dumps

[Takuto Ikuta via Phabricator via llvm-commits]

takuto.ikuta added inline comments.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:28
+
+static std::string escapeString(const char *Src) {
+  std::string OS;
----------------
you can pass StringRef here and remove .data() or .c_str() from caller.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:33
+    switch (C) {
+    case '"':
+    case '\\':
----------------
Include escape for '/' too.
https://tools.ietf.org/html/rfc8259#section-7


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+    default:
+      if (C >= 32 && C < 126) {
+        OS += C;
----------------
I prefer to use isPrint here.
https://github.com/llvm/llvm-project/blob/2946cd701067404b99c39fb29dc9c74bd7193eb3/llvm/include/llvm/ADT/StringExtras.h#L105


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:72
+    Entry E = {steady_clock::now(), {}, Name, Detail};
+    Stack.emplace_back(E);
+  }
----------------
I prefer to write
```
Stack.push_back(std::move(E));
```
or
```
Stack.emplace_back(steady_clock::now(), {}, Name, Detail);
```


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+    *OS << "{ \"traceEvents\": [\n";
+
----------------
Don't we want to use json utility for this?
https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/JSON.h



================
Comment at: llvm/lib/Support/TimeProfiler.cpp:147
+  std::unordered_map<std::string, DurationType> TotalPerName;
+  std::unordered_map<std::string, size_t> CountPerName;
+  time_point<steady_clock> StartTime;
----------------
better to have one hash map so that we don't need to call 2 lookup in L92 and L93.
Also StringMap may be faster than unordered_map.


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

https://reviews.llvm.org/D58675