[Lldb-commits] [PATCH] D32823: Remove an expensive lock from Timer

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, then let's keep them. I don't mind changing all call sites -- having a separate category object is the cleanest solution with least magic. However see my comments about category naming and merging.



================
Comment at: unittests/Core/TimerTest.cpp:39
     std::this_thread::sleep_for(std::chrono::milliseconds(10));
-    Timer t2("CAT1", "");
+    // Explicitly testing the same category as above.
+    static Timer::Category tcat1b("CAT1");
----------------
Do we actually need to support that? Since we already have a nice Category object then I think we should use that as a unique key for everything (including printing). There no reason why the category needs to have local scope. If it really needs to be used from multiple places, we can expose the object at a higher scope. For the tests, this could be done by having a fixture which creates the categories in the SetUpTestCase static function. Alternatively, we could have Category object be based on llvm::ManagedStatic, so it would go away when you call llvm_shutdown, which would enable us to have truly isolated and leak-free tests. (Right now we don't call llvm_shutdown in the main app as we cannot shutdown cleanly, but it would be great if one day we could.)


================
Comment at: unittests/Core/TimerTest.cpp:48
+  // It should only appear once.
+  ASSERT_EQ(nullptr, strstr(strstr(ss.GetData(), "CAT1") + 1, "CAT1"));
   ASSERT_EQ(1, sscanf(ss.GetData(), "%lf sec for CAT1", &seconds));
----------------
ss.GetStringRef().count("CAT1") == 1


Repository:
  rL LLVM

https://reviews.llvm.org/D32823