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

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev marked 3 inline comments as done.
anton-afanasyev added inline comments.


================
Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+    llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
     P.Initialize();
----------------
takuto.ikuta wrote:
> Remove StringRef?
I use `StringRef("")` to explicitly cast to one of overloaded `TimeScope(StringRef, StringRef)` and `TimeScope(StringRef, fuction_ref(std::string()))`.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+    *OS << "{ \"traceEvents\": [\n";
+
----------------
takuto.ikuta wrote:
> Don't we want to use json utility for this?
> https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/JSON.h
> 
This implementation looks compact and fast. I've read proposal for this library https://docs.google.com/document/d/1OEF9IauWwNuSigZzvvbjc1cVS1uGHRyGTXaoy3DjqM4 , it's recent, so I'm not sure it's stable and speed optimized. Do you actually can advise it? I can do it in the next refactor commit as well.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+    default:
+      if (isPrint(C)) {
+        OS += C;
----------------
takuto.ikuta wrote:
> include StringExtras.h for this?
Should one do it? It's already implicitly included.


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

https://reviews.llvm.org/D58675