Swift: address logging review comments

Author: redsun82

swift/README.md

@@ -50,14 +50,15 @@ You can also run `../misc/codegen/codegen.py`, as long as you are beneath the `s
 A log file is produced for each run under `CODEQL_EXTRACTOR_SWIFT_LOG_DIR` (the usual DB log directory).
 
 You can use the environment variable `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` to configure levels for
-loggers and outputs. This must have the form of a comma separated `spec:level` list, where
+loggers and outputs. This must have the form of a comma separated `spec:min_level` list, where
 `spec` is either a glob pattern (made up of alphanumeric, `/`, `*` and `.` characters) for
-matching logger names or one of `out:bin`, `out:text` or `out:console`, and `level` is one of `trace`, `debug`, `info`,
-`warning`, `error`, `critical` or `no_logs` to turn logs completely off.
+matching logger names or one of `out:bin`, `out:text` or `out:console`, and `min_level` is one
+of `trace`, `debug`, `info`, `warning`, `error`, `critical` or `no_logs` to turn logs completely off.
+
 Current output default levels are no binary logs, `info` logs or higher in the text file and `warning` logs or higher on
-standard error. By default, all loggers are configured with the lowest output level. Logger names are visible in the
-textual logs between `[...]`. Examples are `extractor/dispatcher` or `extractor/<source filename>.trap`. An example
-of `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` usage is the following:
+standard error. By default, all loggers are configured with the lowest logging level of all outputs (`info` by default).
+Logger names are visible in the textual logs between `[...]`. Examples are `extractor/dispatcher`
+or `extractor/<source filename>.trap`. An example of `CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS` usage is the following:
 
 ```bash
 export CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS=out:console:trace,out:text:no_logs,*:warning,*.trap:trace
@@ -109,4 +110,5 @@ In particular for breakpoints to work you might need to setup the following remo
 | `bazel-out` | `/absolute/path/to/codeql/bazel-out` |
 
 ### Thread safety
+
 The extractor is single-threaded, and there was no effort to make anything in it thread-safe.

swift/extractor/infra/SwiftDispatcher.h

@@ -152,11 +152,7 @@ class SwiftDispatcher {
       return *l;
     }
     waitingForNewLabel = e;
-    // TODO: more generic and informational visiting one-line log
-    if constexpr (std::is_convertible_v<E, const swift::ValueDecl*>) {
-      const swift::ValueDecl* x = e;
-      LOG_TRACE("{}", x->getName().getBaseIdentifier().str());
-    }
+    // TODO: add tracing logs for visited stuff, maybe within the translators?
     visit(e, std::forward<Args>(args)...);
     Log::flush();
     // TODO when everything is moved to structured C++ classes, this should be moved to createEntry

swift/extractor/infra/log/SwiftLogging.cpp

@@ -23,11 +23,11 @@ Log::Level getLevelFor(std::string_view name, const LevelRules& rules, Log::Leve
   return dflt;
 }
 
-const char* getEnvOr(const char* var, const char* deflt) {
+const char* getEnvOr(const char* var, const char* dflt) {
   if (const char* ret = getenv(var)) {
     return ret;
   }
-  return deflt;
+  return dflt;
 }
 
 std::string_view matchToView(std::csub_match m) {
@@ -48,16 +48,11 @@ Log::Level matchToLevel(std::csub_match m) {
   return stringToLevel(matchToView(m));
 }
 
-struct LevelConfiguration {
-  LevelRules& sourceRules;
-  Log::Level& binSeverity;
-  Log::Level& textSeverity;
-  Log::Level& consoleSeverity;
-  std::vector<std::string>& problems;
-};
+}  // namespace
 
-void collectSeverityRules(const char* var, LevelConfiguration&& configuration) {
-  if (auto levels = getEnvOr(var, nullptr)) {
+std::vector<std::string> Log::collectSeverityRulesAndReturnProblems(const char* envVar) {
+  std::vector<std::string> problems;
+  if (auto levels = getEnvOr(envVar, nullptr)) {
     // expect comma-separated <glob pattern>:<log severity>
     std::regex comma{","};
     std::regex levelAssignment{R"((?:([*./\w]+)|(?:out:(bin|text|console))):()" LEVEL_REGEX_PATTERN
@@ -76,31 +71,28 @@ void collectSeverityRules(const char* var, LevelConfiguration&& configuration) {
             pattern.insert(pos, (pattern[pos] == '*') ? "." : "\\");
             pos += 2;
           }
-          configuration.sourceRules.emplace_back(pattern, level);
+          sourceRules.emplace_back(pattern, level);
         } else {
           auto out = matchToView(match[2]);
           if (out == "bin") {
-            configuration.binSeverity = level;
+            binary.level = level;
           } else if (out == "text") {
-            configuration.textSeverity = level;
+            text.level = level;
           } else if (out == "console") {
-            configuration.consoleSeverity = level;
+            console.level = level;
           }
         }
       } else {
-        configuration.problems.emplace_back("Malformed log level rule: " + it->str());
+        problems.emplace_back("Malformed log level rule: " + it->str());
       }
     }
   }
+  return problems;
 }
 
-}  // namespace
-
 void Log::configure() {
   // as we are configuring logging right now, we collect problems and log them at the end
-  std::vector<std::string> problems;
-  collectSeverityRules("CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS",
-                       {sourceRules, binary.level, text.level, console.level, problems});
+  auto problems = collectSeverityRulesAndReturnProblems("CODEQL_EXTRACTOR_SWIFT_LOG_LEVELS");
   if (text || binary) {
     std::filesystem::path logFile = getEnvOr("CODEQL_EXTRACTOR_SWIFT_LOG_DIR", ".");
     logFile /= logRootName;
@@ -144,7 +136,7 @@ void Log::flushImpl() {
 }
 
 Log::LoggerConfiguration Log::getLoggerConfigurationImpl(std::string_view name) {
-  LoggerConfiguration ret{session, logRootName};
+  LoggerConfiguration ret{session, std::string{logRootName}};
   ret.fullyQualifiedName += '/';
   ret.fullyQualifiedName += name;
   ret.level = std::min({binary.level, text.level, console.level});

swift/extractor/infra/log/SwiftLogging.h

@@ -68,7 +68,7 @@
 namespace codeql {
 
 // tools should define this to tweak the root name of all loggers
-extern const char* const logRootName;
+extern const std::string_view logRootName;
 
 // This class is responsible for the global log state (outputs, log level rules, flushing)
 // State is stored in the singleton `Log::instance()`.
@@ -152,6 +152,7 @@ class Log {
   FilteredOutput<binlog::TextOutputStream> text{Level::info, textFile, format};
   FilteredOutput<binlog::TextOutputStream> console{Level::warning, std::cerr, format};
   LevelRules sourceRules;
+  std::vector<std::string> collectSeverityRulesAndReturnProblems(const char* envVar);
 };
 
 // This class represent a named domain-specific logger, responsible for pushing logs using the

swift/extractor/main.cpp

@@ -20,7 +20,7 @@
 
 using namespace std::string_literals;
 
-const char* const codeql::logRootName = "extractor";
+const std::string_view codeql::logRootName = "extractor";
 
 // must be called before processFrontendOptions modifies output paths
 static void lockOutputSwiftModuleTraps(codeql::SwiftExtractorState& state,
@@ -182,6 +182,7 @@ codeql::SwiftExtractorConfiguration configure(int argc, char** argv) {
   return configuration;
 }
 
+// TODO: use `absl::StrJoin` or `boost::algorithm::join`
 static auto argDump(int argc, char** argv) {
   std::string ret;
   for (auto arg = argv + 1; arg < argv + argc; ++arg) {
@@ -192,13 +193,13 @@ static auto argDump(int argc, char** argv) {
   return ret;
 }
 
+// TODO: use `absl::StrJoin` or `boost::algorithm::join`
 static auto envDump(char** envp) {
   std::string ret;
   for (auto env = envp; *env; ++env) {
     ret += *env;
     ret += '\n';
   }
-  ret.pop_back();
   return ret;
 }
 

swift/extractor/trap/TrapLabel.h

@@ -55,6 +55,7 @@ class UntypedTrapLabel {
   size_t strSize() const {
     if (id_ == undefined) return 17;  // #ffffffffffffffff
     if (id_ == 0) return 2;           // #0
+    // TODO: use absl::bit_width or C+20 std::bit_width instead of this ugly formula
     return /* # */ 1 + /* hex digits */ static_cast<size_t>(ceil(log2(id_ + 1) / 4));
   }
 };