Teach Catch2 about LocalizedString and move out of namespace msg. (#387)

* Move LocalizedString out of namespace msg. The namespace msg is necessary for localized stuff to: 1. Give {formatter} parameters pretty short names without stepping on things. 2. To distinguish the localization safe versions and localization unsafe version of things e.g. vcpkg::println vs msg::println.println. LocalizedString doesn't fall into any of these categories, and as we start passing them around in more places adding msg:: is annoying, and encourages "using namespace"s that harm (1) and (2). * Teach Catch2 about LocalizedString. * clang-format
microsoft · Feb 28, 2022 · e3fceab · e3fceab
1 parent e8be5aa
commit e3fceab
Show file tree

Hide file tree

Showing 11 changed files with 142 additions and 107 deletions.
diff --git a/include/vcpkg-test/util.h b/include/vcpkg-test/util.h
@@ -3,6 +3,7 @@
 #include <catch2/catch.hpp>
 
 #include <vcpkg/base/files.h>
+#include <vcpkg/base/messages.h>
 #include <vcpkg/base/pragmas.h>
 
 #include <vcpkg/statusparagraph.h>
@@ -47,6 +48,12 @@ namespace Catch
     {
         static const std::string& convert(const vcpkg::Triplet& triplet) { return triplet.canonical_name(); }
     };
+
+    template<>
+    struct StringMaker<vcpkg::LocalizedString>
+    {
+        static const std::string convert(const vcpkg::LocalizedString& value) { return "LL\"" + value.data() + "\""; }
+    };
 }
 
 namespace vcpkg::Test

diff --git a/include/vcpkg/base/fwd/messages.h b/include/vcpkg/base/fwd/messages.h
@@ -1,5 +1,7 @@
 #pragma once
 
+#include <vcpkg/base/fwd/stringview.h>
+
 namespace vcpkg
 {
 #if defined(_WIN32)
@@ -20,12 +22,10 @@ namespace vcpkg
     };
 #endif
 
-    struct StringView;
+    struct LocalizedString;
 }
 
 namespace vcpkg::msg
 {
-    struct LocalizedString;
-
     void write_unlocalized_text_to_stdout(Color c, vcpkg::StringView sv);
 }
diff --git a/include/vcpkg/base/messages.h b/include/vcpkg/base/messages.h
@@ -10,6 +10,95 @@
 
 #include <string>
 
+namespace vcpkg
+{
+    struct LocalizedString
+    {
+        LocalizedString() = default;
+        operator StringView() const { return m_data; }
+        const std::string& data() const { return m_data; }
+        std::string extract_data() { return std::exchange(m_data, ""); }
+
+        static LocalizedString from_string_unchecked(std::string&& s)
+        {
+            LocalizedString res;
+            res.m_data = std::move(s);
+            return res;
+        }
+
+        LocalizedString& append(const LocalizedString& s)
+        {
+            m_data.append(s.m_data);
+            return *this;
+        }
+
+        LocalizedString& appendnl()
+        {
+            m_data.push_back('\n');
+            return *this;
+        }
+
+        friend const char* to_printf_arg(const LocalizedString& s) { return s.data().c_str(); }
+
+        friend bool operator==(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() == rhs.data();
+        }
+
+        friend bool operator!=(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() != rhs.data();
+        }
+
+        friend bool operator<(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() < rhs.data();
+        }
+
+        friend bool operator<=(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() <= rhs.data();
+        }
+
+        friend bool operator>(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() > rhs.data();
+        }
+
+        friend bool operator>=(const LocalizedString& lhs, const LocalizedString& rhs)
+        {
+            return lhs.data() >= rhs.data();
+        }
+
+    private:
+        std::string m_data;
+
+        // to avoid lock-in on LocalizedString, these are free functions
+        // this allows us to convert to `std::string` in the future without changing All The Code
+        friend LocalizedString& append_newline(LocalizedString&);
+        friend LocalizedString&& append_newline(LocalizedString&& self) { return std::move(append_newline(self)); }
+        friend LocalizedString& appendnl(LocalizedString& self, const LocalizedString& to_append)
+        {
+            return append_newline(self.append(to_append));
+        }
+    };
+}
+
+namespace fmt
+{
+    template<>
+    struct formatter<vcpkg::LocalizedString> : formatter<vcpkg::StringView>
+    {
+        // parse is inherited from formatter<StringView>
+        template<class FormatContext>
+        auto format(const vcpkg::LocalizedString& s, FormatContext& ctx)
+        {
+            return formatter<vcpkg::StringView>::format(s.data(), ctx);
+        }
+    };
+
+}
+
 namespace vcpkg::msg
 {
     namespace detail
@@ -61,55 +150,6 @@ namespace vcpkg::msg
     // initialize without any localized messages (use default messages only)
     void threadunsafe_initialize_context();
 
-    struct LocalizedString
-    {
-        LocalizedString() = default;
-        operator StringView() const { return m_data; }
-        const std::string& data() const { return m_data; }
-        std::string extract_data() { return std::exchange(m_data, ""); }
-
-        static LocalizedString from_string_unchecked(std::string&& s)
-        {
-            LocalizedString res;
-            res.m_data = std::move(s);
-            return res;
-        }
-
-        LocalizedString& append(const LocalizedString& s)
-        {
-            m_data.append(s.m_data);
-            return *this;
-        }
-        LocalizedString& appendnl()
-        {
-            m_data.push_back('\n');
-            return *this;
-        }
-
-    private:
-        std::string m_data;
-
-        // to avoid lock-in on LocalizedString, these are free functions
-        // this allows us to convert to `std::string` in the future without changing All The Code
-        friend LocalizedString& append_newline(LocalizedString&);
-        friend LocalizedString&& append_newline(LocalizedString&& self) { return std::move(append_newline(self)); }
-        friend LocalizedString& appendnl(LocalizedString& self, const LocalizedString& to_append)
-        {
-            return append_newline(self.append(to_append));
-        }
-    };
-
-    inline const char* to_printf_arg(const msg::LocalizedString& s) { return s.data().c_str(); }
-
-    struct LocalizedStringMapLess
-    {
-        using is_transparent = void;
-        bool operator()(const LocalizedString& lhs, const LocalizedString& rhs) const
-        {
-            return lhs.data() < rhs.data();
-        }
-    };
-
     template<class Message, class... Tags, class... Ts>
     LocalizedString format(Message, detail::MessageArgument<Tags, Ts>... args)
     {
@@ -120,19 +160,19 @@ namespace vcpkg::msg
                                         fmt::make_format_args(fmt::arg(Tags::name(), *args.parameter)...));
     }
 
-    inline void println() { write_unlocalized_text_to_stdout(Color::none, "\n"); }
+    inline void println() { msg::write_unlocalized_text_to_stdout(Color::none, "\n"); }
 
-    inline void print(Color c, const LocalizedString& s) { write_unlocalized_text_to_stdout(c, s); }
-    inline void print(const LocalizedString& s) { write_unlocalized_text_to_stdout(Color::none, s); }
+    inline void print(Color c, const LocalizedString& s) { msg::write_unlocalized_text_to_stdout(c, s); }
+    inline void print(const LocalizedString& s) { msg::write_unlocalized_text_to_stdout(Color::none, s); }
     inline void println(Color c, const LocalizedString& s)
     {
-        write_unlocalized_text_to_stdout(c, s);
-        write_unlocalized_text_to_stdout(Color::none, "\n");
+        msg::write_unlocalized_text_to_stdout(c, s);
+        msg::write_unlocalized_text_to_stdout(Color::none, "\n");
     }
     inline void println(const LocalizedString& s)
     {
-        write_unlocalized_text_to_stdout(Color::none, s);
-        write_unlocalized_text_to_stdout(Color::none, "\n");
+        msg::write_unlocalized_text_to_stdout(Color::none, s);
+        msg::write_unlocalized_text_to_stdout(Color::none, "\n");
     }
 
     template<class Message, class... Ts>
@@ -233,18 +273,3 @@ namespace vcpkg::msg
                     "",
                     "error: cannot specify both --no-{option} and --{option}.");
 }
-
-namespace fmt
-{
-    template<>
-    struct formatter<vcpkg::msg::LocalizedString> : formatter<vcpkg::StringView>
-    {
-        // parse is inherited from formatter<StringView>
-        template<class FormatContext>
-        auto format(const vcpkg::msg::LocalizedString& s, FormatContext& ctx)
-        {
-            return formatter<vcpkg::StringView>::format(s.data(), ctx);
-        }
-    };
-
-}
diff --git a/include/vcpkg/base/parse.h b/include/vcpkg/base/parse.h
@@ -60,9 +60,9 @@ namespace vcpkg::Parse
     struct ParseMessage
     {
         SourceLoc location = {};
-        msg::LocalizedString message;
+        LocalizedString message;
 
-        msg::LocalizedString format(StringView origin, MessageKind kind) const;
+        LocalizedString format(StringView origin, MessageKind kind) const;
     };
 
     struct ParseMessages
@@ -131,11 +131,11 @@ namespace vcpkg::Parse
 
         void add_error(std::string message) { add_error(std::move(message), cur_loc()); }
         void add_error(std::string message, const SourceLoc& loc);
-        void add_error(msg::LocalizedString&& message) { add_error(message.extract_data(), cur_loc()); }
-        void add_error(msg::LocalizedString&& message, const SourceLoc& loc) { add_error(message.extract_data(), loc); }
+        void add_error(LocalizedString&& message) { add_error(message.extract_data(), cur_loc()); }
+        void add_error(LocalizedString&& message, const SourceLoc& loc) { add_error(message.extract_data(), loc); }
 
-        void add_warning(msg::LocalizedString&& message) { add_warning(std::move(message), cur_loc()); }
-        void add_warning(msg::LocalizedString&& message, const SourceLoc& loc);
+        void add_warning(LocalizedString&& message) { add_warning(std::move(message), cur_loc()); }
+        void add_warning(LocalizedString&& message, const SourceLoc& loc);
 
         const IParseError* get_error() const { return m_messages.error.get(); }
         std::unique_ptr<IParseError> extract_error() { return std::move(m_messages.error); }

diff --git a/include/vcpkg/commands.generate-message-map.h b/include/vcpkg/commands.generate-message-map.h
@@ -12,8 +12,8 @@ namespace vcpkg::Commands
         std::vector<StringView> comments_without_argument;
     };
 
-    std::vector<StringView> get_all_format_args(StringView fstring, msg::LocalizedString& error);
-    FormatArgMismatches get_format_arg_mismatches(StringView value, StringView comment, msg::LocalizedString& error);
+    std::vector<StringView> get_all_format_args(StringView fstring, LocalizedString& error);
+    FormatArgMismatches get_format_arg_mismatches(StringView value, StringView comment, LocalizedString& error);
 
     struct GenerateDefaultMessageMapCommand : BasicCommand
     {

diff --git a/include/vcpkg/visualstudio.h b/include/vcpkg/visualstudio.h
@@ -16,7 +16,7 @@ namespace vcpkg
 #if defined(_WIN32)
         std::vector<Path> paths_examined;
         std::vector<Toolset> excluded_toolsets;
-        msg::LocalizedString get_localized_debug_info() const;
+        LocalizedString get_localized_debug_info() const;
 #endif
     };
 }

diff --git a/src/vcpkg-test/messages.cpp b/src/vcpkg-test/messages.cpp
@@ -9,7 +9,7 @@ using namespace vcpkg::Commands;
 
 TEST_CASE ("generate message get_all_format_args", "[messages]")
 {
-    msg::LocalizedString err;
+    LocalizedString err;
     auto res = get_all_format_args("hey ho let's go", err);
     CHECK(err.data() == "");
     CHECK(res == std::vector<StringView>{});
@@ -37,7 +37,7 @@ TEST_CASE ("generate message get_all_format_args", "[messages]")
 
 TEST_CASE ("generate message get_format_arg_mismatches", "[messages]")
 {
-    msg::LocalizedString err;
+    LocalizedString err;
     auto res = get_format_arg_mismatches("hey ho", "", err);
     CHECK(err.data() == "");
     CHECK(res.arguments_without_comment == std::vector<StringView>{});

diff --git a/src/vcpkg/base/messages.cpp b/src/vcpkg/base/messages.cpp
@@ -2,6 +2,15 @@
 #include <vcpkg/base/messages.h>
 #include <vcpkg/base/system.debug.h>
 
+namespace vcpkg
+{
+    LocalizedString& append_newline(LocalizedString& s)
+    {
+        s.m_data.push_back('\n');
+        return s;
+    }
+}
+
 namespace vcpkg::msg
 {
     DECLARE_AND_REGISTER_MESSAGE(NoLocalizationForMessages, (), "", "No localization for the following messages:");
@@ -152,12 +161,6 @@ namespace vcpkg::msg
         }
     }
 
-    LocalizedString& append_newline(LocalizedString& s)
-    {
-        s.m_data.push_back('\n');
-        return s;
-    }
-
     void threadunsafe_initialize_context()
     {
         Messages& m = messages();

diff --git a/src/vcpkg/base/parse.cpp b/src/vcpkg/base/parse.cpp
@@ -30,7 +30,7 @@ namespace vcpkg::Parse
         auto decoder = Unicode::Utf8Decoder(line.data(), line.data() + line.size());
         ParseMessage as_message;
         as_message.location = SourceLoc{std::next(decoder, caret_col), decoder, row, column};
-        as_message.message = msg::LocalizedString::from_string_unchecked(std::string(message));
+        as_message.message = LocalizedString::from_string_unchecked(std::string(message));
 
         auto res = as_message.format(origin, MessageKind::Error).extract_data();
         res.push_back('\n');
@@ -46,12 +46,12 @@ namespace vcpkg::Parse
                                  "Example of {value} is 'x64 & windows'",
                                  "    on expression: {value}");
 
-    msg::LocalizedString ParseMessage::format(StringView origin, MessageKind kind) const
+    LocalizedString ParseMessage::format(StringView origin, MessageKind kind) const
     {
-        msg::LocalizedString res = msg::format(msgFormattedParseMessageLocation,
-                                               msg::path = origin,
-                                               msg::row = location.row,
-                                               msg::column = location.column);
+        LocalizedString res = msg::format(msgFormattedParseMessageLocation,
+                                          msg::path = origin,
+                                          msg::row = location.row,
+                                          msg::column = location.column);
         if (kind == MessageKind::Warning)
         {
             res.append(msg::format(msg::msgWarningMessage));
@@ -88,7 +88,7 @@ namespace vcpkg::Parse
         }
         caret_string.push_back('^');
 
-        res.append(msg::LocalizedString::from_string_unchecked(std::move(caret_string)));
+        res.append(LocalizedString::from_string_unchecked(std::move(caret_string)));
 
         return res;
     }
@@ -128,7 +128,7 @@ namespace vcpkg::Parse
         return cur();
     }
 
-    void ParserBase::add_warning(msg::LocalizedString&& message, const SourceLoc& loc)
+    void ParserBase::add_warning(LocalizedString&& message, const SourceLoc& loc)
     {
         m_messages.warnings.push_back(ParseMessage{loc, std::move(message)});
     }