[PATCH] D131469: [Clang] change default storing path of `-ftime-trace`

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:4514
+// Infer data storing path of the options `-ftime-trace`, `-ftime-trace=<path>`
+void InferTimeTracePath(Compilation &C) {
+  bool HasTimeTrace =
----------------
Use `static`. See https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Use `lowerCase` style for new function names. Ignore existing inconsistency in Clang.


================
Comment at: clang/lib/Driver/Driver.cpp:4516
+  bool HasTimeTrace =
+      C.getArgs().getLastArg(options::OPT_ftime_trace) != nullptr;
+  bool HasTimeTraceFile =
----------------
`hasArg` or `hasArgNoClaim`

The former claims the option so that it won't trigger a -Wunused-command-line-argument diagnostic.


================
Comment at: clang/lib/Driver/Driver.cpp:4529
+  SmallString<128> TracePath("");
+
+  if (HasTimeTraceFile) {
----------------
delete blank line


================
Comment at: clang/lib/Driver/Driver.cpp:4567
+          llvm::sys::path::replace_extension(OutputPath, "json");
+          arg += std::string(OutputPath.c_str());
+        } else {
----------------
This means `arg` probably should be a `SmallString` as well to prevent unneeded `std::string` construction.


================
Comment at: clang/lib/Driver/Driver.cpp:4589
+      const std::string::size_type size = arg.size();
+      char *buffer = new char[size + 1];
+      memcpy(buffer, arg.c_str(), size + 1);
----------------
memory leak. try avoiding raw memory allocation.


================
Comment at: clang/lib/Driver/Driver.cpp:4595
+      auto &JArgs = J.getArguments();
+      for (unsigned I = 0; I < JArgs.size(); ++I) {
+        if (StringRef(JArgs[I]).startswith("-ftime-trace=") ||
----------------
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop


================
Comment at: clang/lib/Driver/Driver.cpp:4597
+        if (StringRef(JArgs[I]).startswith("-ftime-trace=") ||
+            (StringRef(JArgs[I]).equals("-ftime-trace") && !HasTimeTraceFile)) {
+          ArgStringList NewArgs(JArgs.begin(), JArgs.begin() + I);
----------------
Prefer `==` to equals


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131469