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

[Takuto Ikuta via Phabricator via cfe-commits]

takuto.ikuta added inline comments.


================
Comment at: clang/lib/Parse/ParseAST.cpp:154
   if (HaveLexer) {
+    llvm::TimeTraceScope TimeScope("Frontend", StringRef(""));
     P.Initialize();
----------------
anton-afanasyev wrote:
> 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()))`.
I think function_ref(std::string()) does not have constructor receiving StringRef, so I feel explicit cast is redundant.



================
Comment at: llvm/lib/Support/TimeProfiler.cpp:103
+
+    *OS << "{ \"traceEvents\": [\n";
+
----------------
anton-afanasyev wrote:
> 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.
Hmm, I think using json library is preferred because we don't need to take care the correctness of json format.
Confirming correctness of json format with many escaped literals is bit hard and I'm afraid to miss json format error.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:44
+    default:
+      if (isPrint(C)) {
+        OS += C;
----------------
anton-afanasyev wrote:
> takuto.ikuta wrote:
> > include StringExtras.h for this?
> Should one do it? It's already implicitly included.
I think it is better to do, because if someone removes the StringExtras.h include used for this file, they need to include header in this file at the same time. That may require to touch unrelated files in their change.


================
Comment at: llvm/lib/Support/TimeProfiler.cpp:75
+  void end() {
+    assert(!Stack.empty() && "Must call Begin first");
+    auto &E = Stack.back();
----------------
s/Begin/begin/


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

https://reviews.llvm.org/D58675