Swift: refactor after review

swift/log/BUILD.bazel

@@ -4,6 +4,7 @@ cc_library(
     hdrs = glob(["*.h"]),
     visibility = ["//visibility:public"],
     deps = [
+        "@absl//absl/strings",
         "@binlog",
         "@json",
     ],

swift/log/SwiftDiagnostics.cpp

@@ -2,57 +2,41 @@
 
 #include <binlog/Entries.hpp>
 #include <nlohmann/json.hpp>
+#include "absl/strings/str_join.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/str_split.h"
 
 namespace codeql {
-SwiftDiagnosticsSource::SwiftDiagnosticsSource(std::string_view internalId,
-                                               std::string&& name,
-                                               std::vector<std::string>&& helpLinks,
-                                               std::string&& action)
-    : name{std::move(name)}, helpLinks{std::move(helpLinks)}, action{std::move(action)} {
-  id = extractorName;
-  id += '/';
-  id += programName;
-  id += '/';
-  std::transform(internalId.begin(), internalId.end(), std::back_inserter(id),
-                 [](char c) { return c == '_' ? '-' : c; });
-}
-
-void SwiftDiagnosticsSource::create(std::string_view id,
-                                    std::string name,
-                                    std::vector<std::string> helpLinks,
-                                    std::string action) {
-  auto [it, inserted] = map().emplace(
-      id, SwiftDiagnosticsSource{id, std::move(name), std::move(helpLinks), std::move(action)});
-  assert(inserted);
-}
-
 void SwiftDiagnosticsSource::emit(std::ostream& out,
                                   std::string_view timestamp,
                                   std::string_view message) const {
-  nlohmann::json entry;
-  auto& source = entry["source"];
-  source["id"] = id;
-  source["name"] = name;
-  source["extractorName"] = extractorName;
-
-  auto& visibility = entry["visibility"];
-  visibility["statusPage"] = true;
-  visibility["cliSummaryTable"] = true;
-  visibility["telemetry"] = true;
-
-  entry["severity"] = "error";
-  entry["helpLinks"] = helpLinks;
-  std::string plaintextMessage{message};
-  plaintextMessage += ".\n\n";
-  plaintextMessage += action;
-  plaintextMessage += '.';
-  entry["plaintextMessage"] = plaintextMessage;
-
-  entry["timestamp"] = timestamp;
-
+  nlohmann::json entry = {
+      {"source",
+       {
+           {"id", sourceId()},
+           {"name", name},
+           {"extractorName", extractorName},
+       }},
+      {"visibility",
+       {
+           {"statusPage", true},
+           {"cliSummaryTable", true},
+           {"telemetry", true},
+       }},
+      {"severity", "error"},
+      {"helpLinks", std::vector<std::string_view>(absl::StrSplit(helpLinks, ' '))},
+      {"plaintextMessage", absl::StrCat(message, ".\n\n", action, ".")},
+      {"timestamp", timestamp},
+  };
   out << entry << '\n';
 }
 
+std::string SwiftDiagnosticsSource::sourceId() const {
+  auto ret = absl::StrJoin({extractorName, programName, id}, "/");
+  std::replace(ret.begin(), ret.end(), '_', '-');
+  return ret;
+}
+
 void SwiftDiagnosticsDumper::write(const char* buffer, std::size_t bufferSize) {
   binlog::Range range{buffer, bufferSize};
   binlog::RangeEntryStream input{range};
@@ -60,12 +44,11 @@ void SwiftDiagnosticsDumper::write(const char* buffer, std::size_t bufferSize) {
     const auto& source = SwiftDiagnosticsSource::get(event->source->category);
     std::ostringstream oss;
     timestampedMessagePrinter.printEvent(oss, *event, events.writerProp(), events.clockSync());
+    // TODO(C++20) use oss.view() directly
     auto data = oss.str();
     std::string_view view = data;
-    auto sep = view.find(' ');
-    assert(sep != std::string::npos);
-    auto timestamp = view.substr(0, sep);
-    auto message = view.substr(sep + 1);
+    using ViewPair = std::pair<std::string_view, std::string_view>;
+    auto [timestamp, message] = ViewPair(absl::StrSplit(view, absl::MaxSplits(' ', 1)));
     source.emit(output, timestamp, message);
   }
 }

swift/log/SwiftDiagnostics.h

@@ -8,6 +8,8 @@
 #include <cassert>
 #include <fstream>
 #include <filesystem>
+#include <sstream>
+#include <mutex>
 
 namespace codeql {
 
@@ -16,16 +18,27 @@ extern const std::string_view programName;
 // Models a diagnostic source for Swift, holding static information that goes out into a diagnostic
 // These are internally stored into a map on id's. A specific error log can use binlog's category
 // as id, which will then be used to recover the diagnostic source while dumping.
-class SwiftDiagnosticsSource {
- public:
-  // creates a SwiftDiagnosticsSource with the given data
-  static void create(std::string_view id,
-                     std::string name,
-                     std::vector<std::string> helpLinks,
-                     std::string action);
+struct SwiftDiagnosticsSource {
+  std::string_view id;
+  std::string_view name;
+  static constexpr std::string_view extractorName = "swift";
+  std::string_view action;
+  std::string_view helpLinks;  // space separated if more than 1. Not a vector to allow constexpr
 
-  // gets a previously created SwiftDiagnosticsSource for the given id. Will abort if none exists
-  static const SwiftDiagnosticsSource& get(const std::string& id) { return map().at(id); }
+  // for the moment, we only output errors, so no need to store the severity
+
+  // registers a diagnostics source for later retrieval with get, if not done yet
+  template <const SwiftDiagnosticsSource* Spec>
+  static void inscribe() {
+    static std::once_flag once;
+    std::call_once(once, [] {
+      auto [it, inserted] = map().emplace(Spec->id, Spec);
+      assert(inserted);
+    });
+  }
+
+  // gets a previously inscribed SwiftDiagnosticsSource for the given id. Will abort if none exists
+  static const SwiftDiagnosticsSource& get(const std::string& id) { return *map().at(id); }
 
   // emit a JSON diagnostics for this source with the given timestamp and message to out
   // A plaintextMessage is used that includes both the message and the action to take. Dots are
@@ -34,26 +47,13 @@ class SwiftDiagnosticsSource {
   void emit(std::ostream& out, std::string_view timestamp, std::string_view message) const;
 
  private:
-  using Map = std::unordered_map<std::string, SwiftDiagnosticsSource>;
-
-  std::string id;
-  std::string name;
-  static constexpr std::string_view extractorName = "swift";
-
-  // for the moment, we only output errors, so no need to store the severity
-
-  std::vector<std::string> helpLinks;
-  std::string action;
+  std::string sourceId() const;
+  using Map = std::unordered_map<std::string, const SwiftDiagnosticsSource*>;
 
   static Map& map() {
     static Map ret;
     return ret;
   }
-
-  SwiftDiagnosticsSource(std::string_view internalId,
-                         std::string&& name,
-                         std::vector<std::string>&& helpLinks,
-                         std::string&& action);
 };
 
 // An output modeling binlog's output stream concept that intercepts binlog entries and translates
@@ -76,18 +76,12 @@ class SwiftDiagnosticsDumper {
   binlog::PrettyPrinter timestampedMessagePrinter{"%u %m", "%Y-%m-%dT%H:%M:%S.%NZ"};
 };
 
-namespace diagnostics {
-inline void internal_error() {
-  SwiftDiagnosticsSource::create("internal_error", "Internal error", {},
-                                 "Contact us about this issue");
-}
-}  // namespace diagnostics
-
-namespace detail {
-template <void (*Func)()>
-inline void createSwiftDiagnosticsSourceOnce() {
-  static int ignore = (Func(), 0);
-  std::ignore = ignore;
-}
-}  // namespace detail
 }  // namespace codeql
+
+namespace codeql_diagnostics {
+constexpr codeql::SwiftDiagnosticsSource internal_error{
+    "internal_error",
+    "Internal error",
+    "Contact us about this issue",
+};
+}  // namespace codeql_diagnostics

swift/log/SwiftLogging.h

@@ -43,17 +43,15 @@
 
 #define LOG_WITH_LEVEL(LEVEL, ...) LOG_WITH_LEVEL_AND_CATEGORY(LEVEL, , __VA_ARGS__)
 
-// Emit errors with a specified diagnostics ID. This must be the name of a function in the
-// codeql::diagnostics namespace, which must call SwiftDiagnosticSource::create with ID as first
-// argument. This function will be called at most once during the program execution.
-// See codeql::diagnostics::internal_error below as an example.
+// Emit errors with a specified diagnostics ID. This must be the name of a `SwiftDiagnosticsSource`
+// defined in the `codeql_diagnostics` namespace, which must have `id` equal to its name.
 #define DIAGNOSE_CRITICAL(ID, ...) DIAGNOSE_WITH_LEVEL(critical, ID, __VA_ARGS__)
 #define DIAGNOSE_ERROR(ID, ...) DIAGNOSE_WITH_LEVEL(error, ID, __VA_ARGS__)
 
-#define DIAGNOSE_WITH_LEVEL(LEVEL, ID, ...)                                      \
-  do {                                                                           \
-    codeql::detail::createSwiftDiagnosticsSourceOnce<codeql::diagnostics::ID>(); \
-    LOG_WITH_LEVEL_AND_CATEGORY(LEVEL, ID, __VA_ARGS__);                         \
+#define DIAGNOSE_WITH_LEVEL(LEVEL, ID, ...)                              \
+  do {                                                                   \
+    codeql::SwiftDiagnosticsSource::inscribe<&codeql_diagnostics::ID>(); \
+    LOG_WITH_LEVEL_AND_CATEGORY(LEVEL, ID, __VA_ARGS__);                 \
   } while (false)
 
 // avoid calling into binlog's original macros