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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
