[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidy.h:230-232
+/// \param StoreCheckProfile If provided, and EnableCheckProfile is true,
+/// the profile will not be outputted to the stderr, but instead will be stored
+/// as JSON file in the specified directory.
Minor wording nits to fix grammar, how about: "...the profile will not be
output to stderr, but will instead be stored as a JSON file in the specified
directory."
Comment at: clang-tidy/ClangTidyProfiling.cpp:47
+void ClangTidyProfiling::printAsJSON(llvm::raw_ostream ) {
+ OS << "{\n";
I'm not keen that we call this `printAsJSON()` when the docs talk about writing
out YAML. While all JSON is valid YAML, that feels like trivia we shouldn't
encode in the function name. What do you think to renaming to `printAsYAML()`
instead?
Comment at: clang-tidy/ClangTidyProfiling.cpp:77
+
+ printAsJSON(OS);
+}
I'm not keen that we call `printAsJSON` when the docs talk about writing out
YAML. While all JSON is valid YAML, that feels like trivia we shouldn't encode
in the function name. What do you think to renaming to `printAsYAML()` instead?
Comment at: docs/clang-tidy/index.rst:257
+
:program:`clang-tidy` diagnostics are intended to call out code that does
Spurious change?
Comment at: docs/clang-tidy/index.rst:757
+
+To enable profiling info collection, use ``-enable-check-profile`` argument.
+The timings will be outputted to the ``stderr`` as a table. Example output:
use -> use the
Comment at: docs/clang-tidy/index.rst:758
+To enable profiling info collection, use ``-enable-check-profile`` argument.
+The timings will be outputted to the ``stderr`` as a table. Example output:
+
will be output to stderr
Comment at: docs/clang-tidy/index.rst:802
+ Example:
+ Let's suppose you have a source file named ``example.cpp``, located in
+ ``/source`` directory. Only the input filename is used, not the full path
in -> in the
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:342-347
+if (!llvm::sys::fs::exists(AbsolutePath)) {
+ // If the destination prefix does not exist, don't try to use
real_path().
+ return AbsolutePath;
+}
+SmallString<256> dest;
+if (std::error_code EC = llvm::sys::fs::real_path(AbsolutePath, dest)) {
aaron.ballman wrote:
> This creates a TOCTOU bug; can you call `real_path()` without checking for
> existence and instead check the error code to decide whether to spit out an
> error or return `AbsolutePath`?
Hmm, i think this can just go away.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 149610.
lebedev.ri marked 6 inline comments as done.
lebedev.ri added a comment.
Rebased.
Addressed review notes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
aaron.ballman added inline comments.
Comment at: clang-tidy/ClangTidyProfiling.cpp:72
+ if (EC) {
+llvm::errs() << "Error opening output file'" << Storage->StoreFilename
+ << "': " << EC.message() << "\n";
Missing a whitespace before the quote for the file name.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:334
+ auto processPrefix = [](const std::string ) -> SmallString<256> {
+if (input.empty())
The identifiers here don't match the coding convention (should be
`ProcessPrefix` and `Input` by convention).
Comment at: clang-tidy/tool/ClangTidyMain.cpp:342-347
+if (!llvm::sys::fs::exists(AbsolutePath)) {
+ // If the destination prefix does not exist, don't try to use
real_path().
+ return AbsolutePath;
+}
+SmallString<256> dest;
+if (std::error_code EC = llvm::sys::fs::real_path(AbsolutePath, dest)) {
This creates a TOCTOU bug; can you call `real_path()` without checking for
existence and instead check the error code to decide whether to spit out an
error or return `AbsolutePath`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1116973, @george.karpenkov wrote: > LGTM with a nit to me Thank you! I suspect that at least temporarily we will end up with two different tooling sets to further post-process these results, since i really love to write bicycles and then hopefully throw them away :) In https://reviews.llvm.org/D46602#1116973, @george.karpenkov wrote: > but maybe clang-tidy code owners would need to sign that off as well. Yeah, that is time-consuming as usual. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
george.karpenkov accepted this revision. george.karpenkov added a comment. LGTM with a nit to me, but maybe clang-tidy code owners would need to sign that off as well. Comment at: clang-tidy/ClangTidy.h:232 + bool EnableCheckProfile = false, + llvm::StringRef StoreCheckProfile = StringRef()); doxygen comment for new param? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. Ping. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:188
+format to stderr. When this option is passed,
+these per-TU profiles are instead stored as YAML.)"),
+ cl::value_desc("prefix"),
Quuxplusone wrote:
> Drive-by nit: Each other help string ends with a newline (`)"),` on a line by
> itself). This one presumably should too.
Hmm, yeah, at least for consistency.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
Quuxplusone added inline comments.
Comment at: clang-tidy/tool/ClangTidyMain.cpp:188
+format to stderr. When this option is passed,
+these per-TU profiles are instead stored as YAML.)"),
+ cl::value_desc("prefix"),
Drive-by nit: Each other help string ends with a newline (`)"),` on a line by
itself). This one presumably should too.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 147120.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.
Rename getter/setter functions.
Thank you for taking a look!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh added a comment. A couple of comments from a cursory look. I'll try to look closer later this week. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:173 + /// \brief Control storage of profile date. + void setStoreProfile(StringRef ProfilePrefix); + llvm::Optional getStoreProfile() const; The name is misleading. I'd suggest using a name that unambiguously states that we're setting a prefix for profiling output files. Some ideas: setProfilePathPrefix, setStoredProfilePathPrefix, setProfileStorePathPrefix, setProfileResultsPathPrefix. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:174 + void setStoreProfile(StringRef ProfilePrefix); + llvm::Optional getStoreProfile() const; + getProfileStorageParams? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 147071.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.
- Rebased
- Slightly refactor how the profile storage params are computed, move that into
`ClangTidyProfiling::StorageParams` helper struct.
- Store timestamp in the json too, since it's already in the filename. May be
useful for garbage collection and alike.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,37 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "timestamp": "{{[0-9]+}}-{{[0-9]+}}-{{[0-9]+}} {{[0-9]+}}:{{[0-9]+}}:{{[0-9]+}}.{{[0-9]+}}",
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT:
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a project: clang-tools-extra. lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1098092, @alexfh wrote: > In https://reviews.llvm.org/D46602#1097954, @lebedev.ri wrote: > > > How do i reflect that in tests? The output name will basically be random. > > > Why random? I was obviously talking about the FileCheck stuff, how to specify such a dynamic filename there.. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 146644.
lebedev.ri edited the summary of this revision.
lebedev.ri added subscribers: aaron.ballman, JonasToth.
lebedev.ri added a comment.
- Get rid of 'prefix elision'
- Don't use full source file name, only the filename
- Prefix that filename with ISO8601-like timestamp of the profile creation.
I think this is worse than before, horrible and ugly, but hey, i'm not the code
owner.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,35 @@
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+// RUN: rm -rf %T/out
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: cat %T/out/*-clang-tidy-store-check-profile-one-tu.cpp.yaml | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}}
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh added a comment. In https://reviews.llvm.org/D46602#1097954, @lebedev.ri wrote: > In https://reviews.llvm.org/D46602#1097902, @alexfh wrote: > > > In https://reviews.llvm.org/D46602#1095980, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D46602#1095960, @alexfh wrote: > > > > > > > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > > > > > > > > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > > > > > > > > > Roman, it looks to me that a simpler storage scheme would be > > > > > > sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv. > > > > > > Main things are: > > > > > > > > > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > > > > > > > > > > > > > Of the input source file? > > > > > > > > > > > > No, current timestamp, when each profile gets dumped. > > > > > > > > > Ah. > > > Then i don't understand this at all. Why would we want to do that, > > > exactly? > > > Just so that we can avoid creating directory structure? Why do we want > > > to avoid that? > > > > > > If clang-tidy is invoked manually, a simpler naming scheme with less > > configuration options would be easier to use, in particular: > > > > 1. no the need to specify the `-store-check-profile-elide-prefix=` option; > > 2. it's easier to see all results (no need to use `find`, just `ls > > /output/directory` or `ls /output/directory/20180514*` to see today's > > results, for example); > > 3. no chance of filename collision, and thus no chance of losing older > > results by just running clang-tidy again. > > > How do i reflect that in tests? The output name will basically be random. Why random? It will follow a certain rule that can be verified. If we decide to name the file `MMDD_InputFileName.cpp.yaml`, for example, then the test can verify the file matching `_InputFileName.cpp.yaml` exists and contains whatever it should contain. If we want the test to be stricter, it could verify that the `` part matches a certain pattern (say, `20\d\d[0-1][0-9][0-3][0-9]`). Additionally, it could verify that after a short sleep a file with a different `` part is generated. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1097902, @alexfh wrote: > In https://reviews.llvm.org/D46602#1095980, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D46602#1095960, @alexfh wrote: > > > > > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > > > > > > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > > > > > > > Roman, it looks to me that a simpler storage scheme would be > > > > > sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv. > > > > > Main things are: > > > > > > > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > > > > > > > > > > Of the input source file? > > > > > > > > > No, current timestamp, when each profile gets dumped. > > > > > > Ah. > > Then i don't understand this at all. Why would we want to do that, exactly? > > Just so that we can avoid creating directory structure? Why do we want to > > avoid that? > > > If clang-tidy is invoked manually, a simpler naming scheme with less > configuration options would be easier to use, in particular: > > 1. no the need to specify the `-store-check-profile-elide-prefix=` option; > 2. it's easier to see all results (no need to use `find`, just `ls > /output/directory` or `ls /output/directory/20180514*` to see today's > results, for example); > 3. no chance of filename collision, and thus no chance of losing older > results by just running clang-tidy again. How do i reflect that in tests? The output name will basically be random. > If clang-tidy is started by a script (e.g. run-clang-tidy.py or a > modification of it specialized on profiling), then a specific naming scheme > doesn't matter much, since the script can create a separate directory for > each run or for each shard. I don't yet see how repeating the directory > structure of the input files would make profiling results easier or more > convenient to use. A relation between the input file and the result file can > still be maintained in some other form (for example, by specifying the whole > invocation inside the results file - if we want to use YAML). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. In https://reviews.llvm.org/D46602#1095980, @lebedev.ri wrote: > In https://reviews.llvm.org/D46602#1095960, @alexfh wrote: > > > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > > > > > Roman, it looks to me that a simpler storage scheme would be > > > > sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv. > > > > Main things are: > > > > > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > > > > > > > Of the input source file? > > > > > > No, current timestamp, when each profile gets dumped. > > > Ah. > Then i don't understand this at all. Why would we want to do that, exactly? > Just so that we can avoid creating directory structure? Why do we want to > avoid that? If clang-tidy is invoked manually, a simpler naming scheme with less configuration options would be easier to use, in particular: 1. no the need to specify the `-store-check-profile-elide-prefix=` option; 2. it's easier to see all results (no need to use `find`, just `ls /output/directory` or `ls /output/directory/20180514*` to see today's results, for example); 3. no chance of filename collision, and thus no chance of losing older results by just running clang-tidy again. If clang-tidy is started by a script (e.g. run-clang-tidy.py or a modification of it specialized on profiling), then a specific naming scheme doesn't matter much, since the script can create a separate directory for each run or for each shard. I don't yet see how repeating the directory structure of the input files would make profiling results easier or more convenient to use. A relation between the input file and the result file can still be maintained in some other form (for example, by specifying the whole invocation inside the results file - if we want to use YAML). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1095960, @alexfh wrote: > In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > > > Roman, it looks to me that a simpler storage scheme would be sufficient. > > > For example, MMDDhhmmss-InputFileName.cpp.csv. > > > Main things are: > > > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > > > > Of the input source file? > > > No, current timestamp, when each profile gets dumped. Ah. Then i don't understand this at all. Why would we want to do that, exactly? Just so that we can avoid creating directory structure? Why do we want to avoid that? >>> 2. include just the name of the file without any parent directories, >> >> That won't work, there are duplicate filenames even in LLVM. > > That's why I suggested to include a timestamp. Each result file will get a > unique timestamp as a part of its name (unless the resolution of the > timestamp is too coarse - compared to clang-tidy run time - to allow > collisions). > >> $ find -iname Path.cpp >> ./lib/Support/Path.cpp >> ./unittests/Support/Path.cpp >> >> > 3. put all outputs into the same directory. This way we wouldn't have to >> create a directory structure and think about stripping a certain prefix >> (btw, utilities like patch just specify the number of path components to >> remove from the start, not the actual substring). WDYT? >> >>I'm not particularly looking forward to having being forced to have n >> thousands of reports in a single directory :) > > What kind of problems do you expect to have with this? Are you going to look > at the result files manually or use a script to aggregate them? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh added a comment. In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > Roman, it looks to me that a simpler storage scheme would be sufficient. > > For example, MMDDhhmmss-InputFileName.cpp.csv. > > Main things are: > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > Of the input source file? No, current timestamp, when each profile gets dumped. >> 2. include just the name of the file without any parent directories, > > That won't work, there are duplicate filenames even in LLVM. That's why I suggested to include a timestamp. Each result file will get a unique timestamp as a part of its name (unless the resolution of the timestamp is too coarse - compared to clang-tidy run time - to allow collisions). > $ find -iname Path.cpp > ./lib/Support/Path.cpp > ./unittests/Support/Path.cpp > > > 3. put all outputs into the same directory. This way we wouldn't have to > create a directory structure and think about stripping a certain prefix (btw, > utilities like patch just specify the number of path components to remove > from the start, not the actual substring). WDYT? > >I'm not particularly looking forward to having being forced to have n > thousands of reports in a single directory :) What kind of problems do you expect to have with this? Are you going to look at the result files manually or use a script to aggregate them? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 145995.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.
- Make json less flat, store source filename in it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,33 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT:"file": "{{.*}}clang-tidy-store-check-profile-one-tu.cpp",
+// CHECK-FILE-NEXT:"profile": {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NEXT: }
+// CHECK-FILE-NEXT: }
+
+// CHECK-FILE-NOT: {
+// CHECK-FILE-NOT: "file": {{.*}}clang-tidy-store-check-profile-one-tu.cpp{{.*}},
+// CHECK-FILE-NOT: "profile": {
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.user": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE-NOT: "time.clang-tidy.readability-function-size.sys": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}}
+// CHECK-FILE-NOT: }
+// CHECK-FILE-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK-NOT: ===-===
+// CHECK-NOT: clang-tidy checks
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1092902, @lebedev.ri wrote: > In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > > > Roman, it looks to me that a simpler storage scheme would be sufficient. > > For example, MMDDhhmmss-InputFileName.cpp.csv. > > Main things are: > > > > 1. include a timestamp, so there's no need to overwrite old results, > > > Of the input source file? > > I don't like this, because when working on trying to improve the performance > of the check, > one wouldn't touch the source file, only the clang-tidy sources. So either > you would > have to `touch` sources (and if they are not writable?), or remove `.csv` > beforehand, > or not output to file, but redirect output. ... also, a new report with a new name will be created each time the filetime changes, so not only will it be fun from tooling point of view, but it will also leave old reports in-place.. > Neither of these possibilities sound great to me. > >> 2. include just the name of the file without any parent directories, > > That won't work, there are duplicate filenames even in LLVM. > > $ find -iname Path.cpp > ./lib/Support/Path.cpp > ./unittests/Support/Path.cpp > > > > >> 3. put all outputs into the same directory. This way we wouldn't have to >> create a directory structure and think about stripping a certain prefix >> (btw, utilities like patch just specify the number of path components to >> remove from the start, not the actual substring). WDYT? > > I'm not particularly looking forward to having being forced to have n > thousands of reports in a single directory :) Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1092890, @alexfh wrote: > Roman, it looks to me that a simpler storage scheme would be sufficient. For > example, MMDDhhmmss-InputFileName.cpp.csv. > Main things are: > > 1. include a timestamp, so there's no need to overwrite old results, Of the input source file? I don't like this, because when working on trying to improve the performance of the check, one wouldn't touch the source file, only the clang-tidy sources. So either you would have to `touch` sources (and if they are not writable?), or remove `.csv` beforehand, or not output to file, but redirect output. Neither of these possibilities sound great to me. > 2. include just the name of the file without any parent directories, That won't work, there are duplicate filenames even in LLVM. $ find -iname Path.cpp ./lib/Support/Path.cpp ./unittests/Support/Path.cpp > 3. put all outputs into the same directory. This way we wouldn't have to > create a directory structure and think about stripping a certain prefix (btw, > utilities like patch just specify the number of path components to remove > from the start, not the actual substring). WDYT? I'm not particularly looking forward to having being forced to have n thousands of reports in a single directory :) Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added a comment. In https://reviews.llvm.org/D46602#1092883, @alexfh wrote: > In https://reviews.llvm.org/D46602#1092111, @rja wrote: > > > +1 for JSON > > > Could you explain why would you use YAML or JSON for this? How are you going > to use/process this data? I personally don't have a preference here. The "YAML" may be better because that is what already supported by `TimerGroup`, which allowed to drop the table printer, too. The next step will be a python script: 1. Takes either a file names (these generated `.yaml`), or a prefix, If it is a prefix, it GLOBS all the `.yaml`'s in there. 2. Load all the files from step 1. 3. Print global report from data collected (`reduce(+)` after grouping by check name) from all the files (what https://reviews.llvm.org/D45931 did) 4. Maybe print report about outliers - without "`reduce(+)` after grouping by check name", with percentages to the total time spent on TU. This is where the filename (and YAML) would be helpful. 5. ??? 6. Run on codebases, find performance problems, contribute fixes 7. Profit! Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh added a comment. Roman, it looks to me that a simpler storage scheme would be sufficient. For example, MMDDhhmmss-InputFileName.cpp.csv. Main things are: 1. include a timestamp, so there's no need to overwrite old results, 2. include just the name of the file without any parent directories, 3. put all outputs into the same directory. This way we wouldn't have to create a directory structure and think about stripping a certain prefix (btw, utilities like patch just specify the number of path components to remove from the start, not the actual substring). WDYT? Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
alexfh added a comment. In https://reviews.llvm.org/D46602#1092111, @rja wrote: > +1 for JSON Could you explain why would you use YAML or JSON for this? How are you going to use/process this data? Repository: rL LLVM https://reviews.llvm.org/D46602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri added inline comments.
Comment at: docs/clang-tidy/index.rst:783
+ {
+"time.clang-tidy.readability-function-size.wall": 1.0421266555786133e+00,
+"time.clang-tidy.readability-function-size.user": 9.208840005421e-01,
Hmm, thinking about it a bit more, i think it should also contain the `"file"`,
```
{
"file": "/source/example.cpp",
"profile": {
"time.clang-tidy.readability-function-size.wall": 9.5367431640625000e-05,
"time.clang-tidy.readability-function-size.user": 7.20002617e-05,
"time.clang-tidy.readability-function-size.sys": 1.8920e-05
}
}
```
Repository:
rL LLVM
https://reviews.llvm.org/D46602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 145906.
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.
Herald added a subscriber: llvm-commits.
- Produce valid-er YAML.
Repository:
rL LLVM
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,23 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: {
+// CHECK-FILE-NEXT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-FILE: }
+
+// CHECK-NOT: {
+// CHECK-NOT: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+// CHECK-NOT: }
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK-NOT: ===-===
+// CHECK-NOT: clang-tidy checks profiling
+// CHECK-NOT: ===-===
+// CHECK-NOT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
// CHECK-NOT: {{.*}} --- Name ---
// CHECK-NOT: {{.*}} readability-function-size
// CHECK-NOT: {{.*}} Total
-// CHECK-NOT: ===-===
class A {
A() {}
Index: test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
@@ -1,16 +1,22 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s 2>&1 | FileCheck
[PATCH] D46602: [clang-tidy] Store checks profiling info as YAML files
lebedev.ri updated this revision to Diff 145901.
lebedev.ri retitled this revision from "[clang-tidy] Store checks profiling
info as CSV files" to "[clang-tidy] Store checks profiling info as YAML files".
lebedev.ri added reviewers: george.karpenkov, NoQ.
lebedev.ri added a comment.
- Deduplicate code by switching to already-existing YAML printer infrastructure
from `TimerGroup`
- Switch to YAML. It's kinda ugly, but maybe better than having to manually
construct CSV.. :/
- When the output prefix does not yet exist, still store the profile as YAML,
don't fail at `make_real`
- When storing profile to file, don't print it to screen. I have tried it, and
it is just too noisy for no apparent benefit.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46602
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyProfiling.cpp
clang-tidy/ClangTidyProfiling.h
clang-tidy/tool/ClangTidyMain.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/index.rst
test/clang-tidy/clang-tidy-enable-check-profile-one-tu.cpp
test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
Index: test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
===
--- /dev/null
+++ test/clang-tidy/clang-tidy-store-check-profile-one-tu.cpp
@@ -0,0 +1,17 @@
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/.yaml -check-prefix=CHECK-FILE %s
+// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' -store-check-profile=%T/out -store-check-profile-elide-prefix=%s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK-CONSOLE %s
+// RUN: FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' -input-file=%T/out/.yaml -check-prefix=CHECK-FILE %s
+
+// CHECK-CONSOLE-NOT: ===-===
+// CHECK-CONSOLE-NOT: {{.*}} --- Name ---
+// CHECK-CONSOLE-NOT: {{.*}} readability-function-size
+// CHECK-CONSOLE-NOT: {{.*}} Total
+// CHECK-CONSOLE-NOT: ===-===
+
+// CHECK-FILE: "time.clang-tidy.readability-function-size.wall": {{.*}}{{[0-9]}}.{{[0-9]+}}e{{[-+]}}{{[0-9]}}{{[0-9]}},
+
+class A {
+ A() {}
+ ~A() {}
+};
Index: test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
===
--- test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
+++ test/clang-tidy/clang-tidy-enable-check-profile-two-tu.cpp
@@ -1,22 +1,31 @@
// RUN: clang-tidy -enable-check-profile -checks='-*,readability-function-size' %s %s 2>&1 | FileCheck --match-full-lines -implicit-check-not='{{warning:|error:}}' %s
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK: ===-===
-// CHECK-NEXT: {{.*}} --- Name ---
+// CHECK-NEXT: clang-tidy checks profiling
+// CHECK-NEXT: ===-===
+// CHECK-NEXT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
+// CHECK: {{.*}} --- Name ---
// CHECK-NEXT: {{.*}} readability-function-size
// CHECK-NEXT: {{.*}} Total
-// CHECK-NEXT: ===-===
// CHECK-NOT: ===-===
+// CHECK-NOT: clang-tidy checks profiling
+// CHECK-NOT: ===-===
+// CHECK-NOT: Total Execution Time: {{.*}} seconds ({{.*}} wall clock)
+
// CHECK-NOT: {{.*}} --- Name ---
// CHECK-NOT: {{.*}} readability-function-size
// CHECK-NOT: {{.*}} Total
-// CHECK-NOT: ===-===
class A {
A() {}
Index:
