Fix code review feedback. Add tests for ContextCache.

Author: BillyONeal

include/vcpkg/base/diagnostics.h

@@ -196,8 +196,6 @@ namespace vcpkg
         DiagnosticContext& status_context;
     };
 
-    struct FullyBufferedDiagnosticContext;
-
     struct DiagnosticOrMessageLine
     {
         DiagnosticOrMessageLine(const DiagnosticLine& dl);

include/vcpkg/base/fwd/diagnostics.h

@@ -16,7 +16,9 @@ namespace vcpkg
     struct DiagnosticLine;
     struct DiagnosticContext;
     struct PrintingDiagnosticContext;
+    struct BasicBufferedDiagnosticContext;
     struct SinkBufferedDiagnosticContext;
+    struct ContextBufferedDiagnosticContext;
     struct FullyBufferedDiagnosticContext;
     struct AttemptDiagnosticContext;
     struct WarningDiagnosticContext;

include/vcpkg/tools.h

@@ -7,9 +7,15 @@
 
 #include <vcpkg/fwd/tools.h>
 
+#include <vcpkg/base/cache.h>
+#include <vcpkg/base/expected.h>
 #include <vcpkg/base/stringview.h>
 
+#include <functional>
+#include <map>
 #include <string>
+#include <tuple>
+#include <utility>
 
 namespace vcpkg
 {
@@ -68,4 +74,55 @@ namespace vcpkg
                                               Path config_path,
                                               Path tools,
                                               RequireExactVersions abiToolVersionHandling);
+
+    template<class Key, class Value, class Compare = std::less<>>
+    struct ContextCache
+    {
+        template<class KeyIsh,
+                 class F,
+                 std::enable_if_t<std::is_constructible_v<Key, const KeyIsh&> &&
+                                      detail::is_callable<Compare&, const Key&, const KeyIsh&>::value,
+                                  int> = 0>
+        const Value* get_lazy(DiagnosticContext& context, const KeyIsh& k, F&& f) const
+        {
+            auto it = m_cache.lower_bound(k);
+            // lower_bound returns the first iterator such that it->first is greater than or equal to than k, so k must
+            // be less than or equal to it->first. If k is greater than it->first, then it must be non-equal so we
+            // have a cache miss.
+            if (it == m_cache.end() || m_cache.key_comp()(k, it->first))
+            {
+                ContextBufferedDiagnosticContext cbdc{context};
+                auto maybe_result = f(cbdc);
+                if (auto success = maybe_result.get())
+                {
+                    it = m_cache.emplace_hint(it,
+                                              std::piecewise_construct,
+                                              std::forward_as_tuple(k),
+                                              std::forward_as_tuple(std::move(*success), expected_left_tag));
+                }
+                else
+                {
+                    it = m_cache.emplace_hint(it,
+                                              std::piecewise_construct,
+                                              std::forward_as_tuple(k),
+                                              std::forward_as_tuple(std::move(cbdc.lines), expected_right_tag));
+                }
+            }
+
+            if (auto success = it->second.get())
+            {
+                return success;
+            }
+
+            for (const auto& line : it->second.error())
+            {
+                context.report(line);
+            }
+
+            return nullptr;
+        }
+
+    private:
+        mutable std::map<Key, ExpectedT<Value, std::vector<DiagnosticLine>>, Compare> m_cache;
+    };
 }

src/vcpkg-test/tools.cpp

@@ -374,3 +374,75 @@ TEST_CASE ("parse_tool_data errors", "[tools]")
               "SHA-512 hash must be 128 characters long and contain only hexadecimal digits");
     }
 }
+
+TEST_CASE ("ContextCache", "[tools]")
+{
+    struct JustOpt
+    {
+        int value;
+        Optional<int> operator()(DiagnosticContext& context) const
+        {
+            context.statusln(LocalizedString::from_raw(fmt::format("The value is {}", value)));
+            return value;
+        }
+    };
+
+    ContextCache<std::string, int> cache;
+
+    // success miss then hit (middle key)
+    FullyBufferedDiagnosticContext fbdc1;
+    const auto* first_ptr = cache.get_lazy(fbdc1, "durian", JustOpt{42});
+    REQUIRE(first_ptr != nullptr);
+    CHECK(*first_ptr == 42);
+    CHECK(fbdc1.to_string() == "The value is 42");
+    // functor should NOT run, status lines not replayed:
+    FullyBufferedDiagnosticContext fbdc1_hit;
+    const auto* hit_ptr = cache.get_lazy(fbdc1_hit, "durian", JustOpt{12345});
+    REQUIRE(hit_ptr == first_ptr);
+    CHECK(fbdc1_hit.empty());
+
+    // insert before existing element
+    FullyBufferedDiagnosticContext fbdc2;
+    const auto* below_ptr = cache.get_lazy(fbdc2, "apple", JustOpt{1729});
+    REQUIRE(below_ptr != nullptr);
+    CHECK(*below_ptr == 1729);
+    CHECK(fbdc2.to_string() == "The value is 1729");
+    FullyBufferedDiagnosticContext fbdc2_hit;
+    const auto* below_hit_ptr = cache.get_lazy(fbdc2_hit, "apple", JustOpt{1});
+    REQUIRE(below_hit_ptr == below_ptr);
+    CHECK(fbdc2_hit.empty());
+
+    // insert above existing element
+    FullyBufferedDiagnosticContext fbdc3;
+    const auto* above_ptr = cache.get_lazy(fbdc3, "melon", JustOpt{1234});
+    REQUIRE(above_ptr != nullptr);
+    CHECK(*above_ptr == 1234);
+    CHECK(fbdc3.to_string() == "The value is 1234");
+    FullyBufferedDiagnosticContext fbdc3_hit;
+    const auto* above_hit_ptr = cache.get_lazy(fbdc3_hit, "melon", JustOpt{2});
+    REQUIRE(above_hit_ptr == above_ptr);
+    CHECK(fbdc3_hit.empty());
+
+    // error case: provider returns empty optional and reports diagnostic; ensure cached error re-reports but provider
+    // not re-run
+    int error_call_count = 0;
+    auto failing_provider = [&error_call_count](DiagnosticContext& ctx) -> Optional<int> {
+        ++error_call_count;
+        ctx.statusln(LocalizedString::from_raw(fmt::format("The number of calls is {}", error_call_count)));
+        ctx.report_error(LocalizedString::from_raw("bad"));
+        return nullopt; // failure
+    };
+    FullyBufferedDiagnosticContext fbdc_err1;
+    const auto* err1 = cache.get_lazy(fbdc_err1, "kiwi", failing_provider);
+    REQUIRE(err1 == nullptr);
+    CHECK(!fbdc_err1.empty());
+    CHECK(fbdc_err1.to_string() == "The number of calls is 1\nerror: bad");
+
+    FullyBufferedDiagnosticContext fbdc_err2;
+    const auto* err2 = cache.get_lazy(fbdc_err2, "kiwi", failing_provider); // should not invoke provider again
+    REQUIRE(err2 == nullptr);
+    CHECK(!fbdc_err2.empty());
+    CHECK(fbdc_err2.to_string() == "error: bad"); // status line not replayed
+    const auto second_error_output = fbdc_err2.to_string();
+    CHECK(error_call_count == 1);
+}

src/vcpkg/tools.cpp

@@ -43,57 +43,6 @@ namespace
         {"netbsd", ToolOs::NetBsd},
         {"solaris", ToolOs::Solaris},
     };
-
-    template<class Key, class Value, class Compare = std::less<>>
-    struct ContextCache
-    {
-        template<class KeyIsh,
-                 class F,
-                 std::enable_if_t<std::is_constructible_v<Key, const KeyIsh&> &&
-                                      detail::is_callable<Compare&, const Key&, const KeyIsh&>::value,
-                                  int> = 0>
-        const Value* get_lazy(DiagnosticContext& context, const KeyIsh& k, F&& f) const
-        {
-            auto it = m_cache.lower_bound(k);
-            // lower_bound returns the first iterator such that it->first is greater than or equal to than k, so k must
-            // be less than or equal to it->first. If k is greater than or equal it->first, then it must be equal so we
-            // have a cache hit.
-            if (it == m_cache.end() || m_cache.key_comp()(k, it->first))
-            {
-                ContextBufferedDiagnosticContext cbdc{context};
-                auto maybe_result = f(context);
-                if (auto success = maybe_result.get())
-                {
-                    it = m_cache.emplace_hint(it,
-                                              std::piecewise_construct,
-                                              std::forward_as_tuple(k),
-                                              std::forward_as_tuple(std::move(*success), expected_left_tag));
-                }
-                else
-                {
-                    it = m_cache.emplace_hint(it,
-                                              std::piecewise_construct,
-                                              std::forward_as_tuple(k),
-                                              std::forward_as_tuple(std::move(cbdc.lines), expected_right_tag));
-                }
-            }
-
-            if (auto success = it->second.get())
-            {
-                return success;
-            }
-
-            for (const auto& line : it->second.error())
-            {
-                context.report(line);
-            }
-
-            return nullptr;
-        }
-
-    private:
-        mutable std::map<Key, ExpectedT<Value, std::vector<DiagnosticLine>>, Compare> m_cache;
-    };
 }
 
 namespace vcpkg