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

[Jamie Schmeiser via Phabricator via cfe-commits]

jamieschmeiser added a comment.

Good progress.



================
Comment at: clang/lib/Driver/Driver.cpp:97
 #include "llvm/Support/raw_ostream.h"
+#include <cassert>
 #include <map>
----------------
This include isn't necessary.  There are asserts already in the file so this is transitively included.


================
Comment at: clang/lib/Driver/Driver.cpp:4668
+  // if `-ftime-trace` or `-ftime-trace=<path>` are specified
+  if (TimeTrace || TimeTraceFile) {
+    SmallString<128> TracePath("");
----------------
I think the quick return style is generally preferred.  In this case, test the negative and return.  It is easier to follow and limits indenting.


================
Comment at: clang/lib/Driver/Driver.cpp:4674
+    for (auto &J : C.getJobs()) {
+      if (J.getSource().getKind() == Action::LinkJobClass &&
+          !J.getOutputFilenames().empty()) {
----------------
Can you have a link job without an output filename?  If not, then just have an assert for !empty.  Again, the negative test and continue might be easier to understand.


================
Comment at: clang/lib/Driver/Driver.cpp:4679
+          TracePath = llvm::sys::path::parent_path(
+              SmallString<128>(J.getOutputFilenames()[0].c_str()));
+        } else {
----------------
you create the same small string twice.  better to create a temporary.


================
Comment at: clang/lib/Driver/Driver.cpp:4689
+      if (J.getSource().getKind() == Action::AssembleJobClass ||
+          J.getSource().getKind() == Action::BackendJobClass) {
+        SmallString<128> OutputPath(J.getOutputFilenames()[0].c_str());
----------------
Again, test negative with continue
Do you also want CompileJobClass?


================
Comment at: clang/lib/Driver/Driver.cpp:4700
+            llvm::sys::path::append(TracePath,
+                                    llvm::sys::path::filename(OutputPath));
+            llvm::sys::path::replace_extension(TracePath, "json");
----------------
You are changing TracePath, which will then be altered when you loop back.  I think you need to use a copy of TracePath.


================
Comment at: clang/lib/Driver/Driver.cpp:4714
+        }
+
+        assert(arg.size() > strlen("-ftime-trace") &&
----------------
Can you compute the full path to the directory before the loop and then just copy it and add the name in the loop for each file?


================
Comment at: clang/lib/Driver/Driver.cpp:4725
+        auto &JArgs = J.getArguments();
+        bool exist = false;
+        for (unsigned I = 0; I < JArgs.size(); ++I) {
----------------
Variables should start with an uppercase letter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131469