LogSeverity and FLAGS_minloglevel

[joeyparrish]

log_severity.h says:

// Abseil flags may be defined with type `LogSeverity`. Dependency layering
// constraints require that the `AbslParseFlag()` overload be declared and
// defined in the flags library itself rather than here. The `AbslUnparseFlag()`
// overload is defined there as well for consistency.
//
// absl::LogSeverity Flag String Representation
//
// An `absl::LogSeverity` has a string representation used for parsing
// command-line flags based on the enumerator name (e.g. `kFatal`) or
// its unprefixed name (without the `k`) in any case-insensitive form. (E.g.
// "FATAL", "fatal" or "Fatal" are all valid.) Unparsing such flags produces an
// unprefixed string representation in all caps (e.g. "FATAL") or an integer.
//
// Additionally, the parser accepts arbitrary integers (as if the type were
// `int`).
//
// Examples:
//
//   --my_log_level=kInfo
//   --my_log_level=INFO
//   --my_log_level=info
//   --my_log_level=0

But FLAGS_minloglevel is defined as int, so the parser won't accept --minloglevel=INFO.

Is there any reason why FLAGS_minloglevel can't be defined as type LogSeverity? It would seem to be backward compatible if it were, since the LogSeverity parser accepts ints.

Thanks!

[joeyparrish]

A quick patch seems to work for me. If there's no reason to avoid it, I can send a PR.

[derekmauro]

The documentation appears to be out of date, but LogSeverity would be the wrong type anyway. min_log_level should be LogSeverityAtLeast. We don't need a PR, there would be additional work to make this work on our end anyway.

[joeyparrish]

I see what you mean. LogSeverityAtLeast is a distinct type from LogSeverity. But they are kept in sync with each other and are compatible. If the goal is to be able to type --minloglevel info (and that is my goal), then you only have to touch ~5 lines of code:

diff --git a/absl/log/flags.cc b/absl/log/flags.cc
index 287b3e96..6682ef2d 100644
--- a/absl/log/flags.cc
+++ b/absl/log/flags.cc
@@ -38,7 +38,8 @@ namespace log_internal {
 namespace {
 
 void SyncLoggingFlags() {
-  absl::SetFlag(&FLAGS_minloglevel, static_cast<int>(absl::MinLogLevel()));
+  absl::SetFlag(&FLAGS_minloglevel,
+                static_cast<absl::LogSeverity>(absl::MinLogLevel()));
   absl::SetFlag(&FLAGS_log_prefix, absl::ShouldPrependLogPrefix());
 }
 
@@ -77,7 +78,7 @@ ABSL_FLAG(int, stderrthreshold,
               absl::GetFlag(FLAGS_stderrthreshold)));
     });
 
-ABSL_FLAG(int, minloglevel, static_cast<int>(absl::LogSeverityAtLeast::kInfo),
+ABSL_FLAG(absl::LogSeverity, minloglevel, absl::LogSeverity::kInfo,
           "Messages logged at a lower level than this don't actually "
           "get logged anywhere")
     .OnUpdate([] {
diff --git a/absl/log/flags_test.cc b/absl/log/flags_test.cc
index 1080ea11..4c1bc503 100644
--- a/absl/log/flags_test.cc
+++ b/absl/log/flags_test.cc
@@ -69,7 +69,7 @@ TEST_F(LogFlagsTest, SetStderrThreshold) {
 
 TEST_F(LogFlagsTest, SetMinLogLevel) {
   absl::SetFlag(&FLAGS_minloglevel,
-                static_cast<int>(absl::LogSeverityAtLeast::kError));
+                absl::LogSeverity::kError);
 
   EXPECT_EQ(absl::MinLogLevel(), absl::LogSeverityAtLeast::kError);
 
@@ -77,7 +77,7 @@ TEST_F(LogFlagsTest, SetMinLogLevel) {
       absl::LogSeverityAtLeast::kWarning);
 
   EXPECT_EQ(absl::GetFlag(FLAGS_minloglevel),
-            static_cast<int>(absl::LogSeverityAtLeast::kWarning));
+            absl::LogSeverity::kWarning);
 }
 
 TEST_F(LogFlagsTest, PrependLogPrefix) {
diff --git a/absl/log/internal/flags.h b/absl/log/internal/flags.h
index c4539785..af717313 100644
--- a/absl/log/internal/flags.h
+++ b/absl/log/internal/flags.h
@@ -24,6 +24,7 @@
 
 #include <string>
 
+#include "absl/base/log_severity.h"
 #include "absl/flags/declare.h"
 
 // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
@@ -40,7 +41,7 @@ ABSL_DECLARE_FLAG(int, stderrthreshold);
 // Log messages at this severity or above are logged; others are discarded.
 // Defaults to `INFO`, i.e. log all severities.  See log_severity.h for numeric
 // values of severity levels.
-ABSL_DECLARE_FLAG(int, minloglevel);
+ABSL_DECLARE_FLAG(absl::LogSeverity, minloglevel);
 
 // If specified in the form file:linenum, any messages logged from a matching
 // location will also include a backtrace.

What do you think? Am I overlooking something that makes this a bad idea? Are there drawbacks to this approach that I missed?