[libcxx-commits] [PATCH] D124740: [libc++] Granularize algorithm benchmarks

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 4 inline comments as done.
philnik added inline comments.


================
Comment at: libcxx/benchmarks/CMakeLists.txt:213
   endif()
-  add_benchmark_test(${test_name} ${test_file})
+  add_benchmark_test(${test_name} ${test_path})
 endforeach()
----------------
Mordante wrote:
> Can you explain why this is done?
Otherwise it would try to compile `make_heap.bench.cpp` and not `algorithms/make_heap.bench.cpp`, which it obviously doesn't find.


================
Comment at: libcxx/benchmarks/algorithms/Common.h:62
   V.resize(N);
-  for (int i = 0; i < N; ++i) {
+  for (unsigned int i = 0; i < N; ++i) {
     V[i] = gas;
----------------
Mordante wrote:
> Please try to do these changes in a separate commit and not while moving code.
> If this is the only change, I won't object.
Yes, this was the only code change (and the `inline` added below). It's just to silence a warning. The warnings was there before, but now it's duplicated for every file in `benchmark/`.


================
Comment at: libcxx/benchmarks/algorithms/make_heap.bench.cpp:29
+};
+} // namespace
+
----------------
Mordante wrote:
> Does an anonymous namespace add value here?
With an anonymous namespace the compiler is much more likely if the function is only used in a single place to inline stuff because it could remove the function then. The cost of inlining an empty functions is -15030, so probably almost no function is emitted when it's in an anonymous namespace and used only once. I don't know if it makes any performance difference in these cases though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124740