Convert LOG_TYPE and LOG_LEVELS to enum class #10182

Pokechu22 · 2021-10-21T19:24:04Z

The main improvement of this is that using an invalid value with INFO_LOG_FMT and such gives the message enum "Common::Log::LogType" has no member "XXX" instead of namespace "Common::Log" has no member "XXX".

I've left the names of the actual enum members alone, because otherwise this would be a fairly massive patch needing to change every log statement (and since it's usually used by a macro, all-caps seems fine).

Pokechu22

These are the changes that do something interesting and might need further review:

Pokechu22 · 2021-10-21T19:34:57Z

Source/Core/Common/Logging/LogManager.cpp

-  m_log[VIDEOINTERFACE] = {"VI", "Video Interface"};
-  m_log[WIIMOTE] = {"Wiimote", "Wii Remote"};
-  m_log[WII_IPC] = {"WII_IPC", "WII IPC"};
+  m_log[int(LogType::ACTIONREPLAY)] = {"ActionReplay", "Action Replay"};


#9718 adds EnumMap, which wraps a std::array keyed by a specific enum. For now I think casts to int are acceptable until that's implemented.

I mean, if the date order is correct on GitHub, it should be the first commit. You could base your branch on that first commit and make use of it in here. Since its a common commit, you don't have to worry about which one is first to be merged (you do have to worry about rebasing correctly, if you rebase either branches)

Cherry-picked. I've dealt with rebasing like this before and it's not too hard (I think git automatically skips it if the commit has no other changes, even if it's been rebased; even if not, it's easy enough to fix with an interactive rebase)

Pokechu22 · 2021-10-21T19:38:41Z

Source/Core/Core/IOS/WFS/WFSI.cpp

@@ -540,7 +540,8 @@ std::optional<IPCReply> WFSIDevice::IOCtl(const IOCtlRequest& request)
    // TODO(wfs): Should be returning an error. However until we have
    // everything properly stubbed it's easier to simulate the methods
    // succeeding.
-    request.DumpUnknown(GetDeviceName(), Common::Log::IOS, Common::Log::LWARNING);
+    request.DumpUnknown(GetDeviceName(), Common::Log::LogType::IOS_WFS,
+                        Common::Log::LogLevel::LWARNING);


The log type here (and in WFSSRV) has been changed from IOS to IOS_WFS to match the other log messages in this file and uses of DumpUnknown in other IOS modules.

Pokechu22 · 2021-10-21T19:39:08Z

Source/Core/Core/IOS/USB/Bluetooth/WiimoteDevice.cpp

-    (void)seq_size;
-  }
+  DEBUG_ASSERT(sequence == SDP_SEQ8);
+  (void)seq_size;


DEBUG_ASSERT already makes the exact same loglevel check, so the check doesn't need to be made again.

Instead of this (void)seq_size (which has been there before, and just de-indented out of the if constexpr; but might as well clean things up while we're here), can we use [[maybe_unused]] const u8 seq_size = attrib_list.Read8(attrib_offset); a few line earlier instead? (...which I cannot put an inline code-comment on, since it isn't part of the diff)

Pokechu22 · 2021-10-21T19:40:24Z

Source/Core/DolphinQt/Config/LogConfigWidget.cpp

-  for (int i = 0; i < Common::Log::NUMBER_OF_LOGS; i++)
+  for (int i = 0; i < static_cast<int>(Common::Log::LogType::NUMBER_OF_LOGS); i++)
  {
-    const auto log_type = static_cast<Common::Log::LOG_TYPE>(i);
+    const auto log_type = static_cast<Common::Log::LogType>(i);


This way of iterating over all log types is a bit jank, but there isn't an easy fix that I know of.

Feels odd that there is nothing (aside from writing an std::initializer_list by hand) to retrieve all values of an enum class. At least I couldn't find anything that wasn't similarly jank; and I have the feeling that there should be a way to do something in a constexpr context.

Something like this might work:

Snippet

template <auto last_member, typename = decltype(last_member)> class EnumElementList { using T = decltype(last_member); static_assert(std::is_enum_v<T>); static constexpr T increment(T t) { return static_cast<T>(static_cast<size_t>(t) + 1); } public: class EnumIterator { friend class EnumElementList; EnumIterator() : t{} {} EnumIterator(T t_) : t(t_) {} T t; public: constexpr EnumIterator& operator++() { t = increment(t); return *this; } constexpr T operator*() const { return t; } constexpr bool operator!=(const EnumIterator& other) const { return t != other.t; } }; static constexpr T last_member = last_member; static constexpr T one_after_last_member = increment(last_member); constexpr EnumIterator begin() const { return EnumIterator{}; } constexpr EnumIterator end() const { return EnumIterator{one_after_last_member}; } };

used as constexpr EnumElementList<LogType::WIIMOTE> LOG_TYPES;.

Pokechu22 · 2021-10-21T19:45:37Z

Source/Core/VideoBackends/Vulkan/VulkanContext.cpp

  if (flags & VK_DEBUG_REPORT_ERROR_BIT_EXT)
-    GENERIC_LOG_FMT(Common::Log::HOST_GPU, Common::Log::LERROR, "{}", log_message);
+    ERROR_LOG_FMT(HOST_GPU, "{}", log_message);
  else if (flags & (VK_DEBUG_REPORT_WARNING_BIT_EXT | VK_DEBUG_REPORT_PERFORMANCE_WARNING_BIT_EXT))
-    GENERIC_LOG_FMT(Common::Log::HOST_GPU, Common::Log::LWARNING, "{}", log_message);
+    WARN_LOG_FMT(HOST_GPU, "{}", log_message);
  else if (flags & VK_DEBUG_REPORT_INFORMATION_BIT_EXT)
-    GENERIC_LOG_FMT(Common::Log::HOST_GPU, Common::Log::LINFO, "{}", log_message);
+    INFO_LOG_FMT(HOST_GPU, "{}", log_message);
  else
-    GENERIC_LOG_FMT(Common::Log::HOST_GPU, Common::Log::LDEBUG, "{}", log_message);
+    DEBUG_LOG_FMT(HOST_GPU, "{}", log_message);


This changes uses of GENERIC_LOG_FMT to the level-specific wrappers. I'm not sure why the level-specific wrappers weren't used before.

BhaaLseN · 2021-10-22T15:55:14Z

Source/Core/Common/Logging/LogManager.cpp

  // Ensure the verbosity level is valid
-  if (verbosity < 1)
-    verbosity = 1;
+  if (verbosity < LogLevel::LNOTICE)
+    verbosity = LogLevel::LNOTICE;
  if (verbosity > MAX_LOGLEVEL)
    verbosity = MAX_LOGLEVEL;


Wondering a bit if this snippet should probably be down in SetLogLevel, just to make sure we don't go overboard if anyone else messes up when calling SetLogLevel. Thoughts?

(Also, does std::clamp handle enums correctly? It could be a more terse way of keeping things in check)

std::clamp does handle enums correctly it seems. I've also moved this into SetLogLevel.

BhaaLseN · 2021-10-22T16:00:39Z

Source/Core/Core/IOS/USB/Bluetooth/WiimoteDevice.cpp

-    (void)seq_size;
-  }
+  DEBUG_ASSERT(sequence == SDP_SEQ8);
+  (void)seq_size;


Instead of this (void)seq_size (which has been there before, and just de-indented out of the if constexpr; but might as well clean things up while we're here), can we use [[maybe_unused]] const u8 seq_size = attrib_list.Read8(attrib_offset); a few line earlier instead? (...which I cannot put an inline code-comment on, since it isn't part of the diff)

BhaaLseN · 2021-10-22T16:18:21Z

Source/Core/DolphinQt/Config/LogConfigWidget.cpp

-  for (int i = 0; i < Common::Log::NUMBER_OF_LOGS; i++)
+  for (int i = 0; i < static_cast<int>(Common::Log::LogType::NUMBER_OF_LOGS); i++)
  {
-    const auto log_type = static_cast<Common::Log::LOG_TYPE>(i);
+    const auto log_type = static_cast<Common::Log::LogType>(i);


Feels odd that there is nothing (aside from writing an std::initializer_list by hand) to retrieve all values of an enum class. At least I couldn't find anything that wasn't similarly jank; and I have the feeling that there should be a way to do something in a constexpr context.

BhaaLseN · 2021-10-22T16:23:01Z

Source/Core/VideoBackends/Vulkan/VulkanLoader.cpp

+  GENERIC_LOG_FMT(Common::Log::LogType::VIDEO, static_cast<Common::Log::LogLevel>(level), "{}",
+                  real_msg);


Unless I missed something, LogVulkanResult is only ever called from the LOG_VULKAN_ERROR macro - with the fixed/hardcoded value 2 (which would be LogLevel::LERROR). Thoughts on dropping the level parameter and just calling ERROR_LOG_FMT instead (or making it an actual LogLevel parameter instead of int as alternative)?

I've changed it to a LogLevel parameter.

leoetlino · 2021-10-23T23:14:21Z

Could you rebase this to make sure it builds fine with the new code that has been added in PR 10127?

Feels odd that there is nothing (aside from writing an std::initializer_list by hand) to retrieve all values of an enum class. At least I couldn't find anything that wasn't similarly jank; and I have the feeling that there should be a way to do something in a constexpr context.

Yeah, C++ infamously lacks support for enum reflection.

leoetlino

Reviewed 6 of 6 files at r1, 46 of 46 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Pokechu22)

Source/Core/Common/EnumMap.h, line 2 at r1 (raw file):

// Copyright 2021 Dolphin Emulator Project
// Licensed under GPLv2+

This should use the new licence header.

Source/Core/Common/EnumMap.h, line 27 at r1 (raw file):

  using T = decltype(last_member);
  static_assert(std::is_enum_v<T>);
  static constexpr size_t _Size = static_cast<size_t>(last_member) + 1;

Names that begin with an underscore followed by an uppercase letter are reserved.

Source/Core/Common/EnumMap.h, line 32 at r1 (raw file):

  constexpr EnumMap() = default;
  constexpr EnumMap(const EnumMap& other) = default;
  EnumMap& operator=(const EnumMap& other) = default;

Can't this also be constexpr?

Source/Core/Common/EnumMap.h, line 34 at r1 (raw file):

  EnumMap& operator=(const EnumMap& other) = default;
  constexpr EnumMap(EnumMap&& other) = default;
  EnumMap& operator=(EnumMap&& other) = default;

constexpr?

Source/Core/Common/EnumMap.h, line 66 at r1 (raw file):

  constexpr V* data() { return m_array.data(); }
  constexpr const V* data() const { return m_array.data(); }

Might be worth adding begin() and end() as well? e.g. constexpr auto begin() const { return m_array.begin(); }

Source/Core/Common/EnumMap.h, line 71 at r1 (raw file):

private:
  std::array<V, _Size> m_array;

Seems safer to value-initialize the array (std::array<V, _Size> m_array{};) in case V is e.g. const char*

Source/Core/Common/TypeUtils.h, line 71 at r1 (raw file):

}  // namespace detail

// Template for checking if Types is count occurences of T.

typo: occurrence

Source/Core/Common/Logging/LogManager.cpp, line 211 at r2 (raw file):

  Config::SetBaseOrCurrent(LOGGER_VERBOSITY, GetLogLevel());

  for (int i = 0; i < static_cast<int>(Common::Log::LogType::NUMBER_OF_LOGS); i++)

If you add begin/end to EnumMap, you can keep the range-based for loop which is less verbose.

Source/Core/Common/Logging/LogManager.cpp, line 265 at r2 (raw file):

std::map<std::string, std::string> LogManager::GetLogTypes()
{
  std::map<std::string, std::string> LogTypes;

Why was this renamed?

Source/Core/Common/Logging/LogManager.cpp, line 267 at r2 (raw file):

  std::map<std::string, std::string> LogTypes;

  for (int i = 0; i < static_cast<int>(Common::Log::LogType::NUMBER_OF_LOGS); i++)

If you add begin/end to EnumMap, you can keep the range-based for loop which is less verbose.

Pokechu22 · 2021-10-24T18:45:54Z

This should use the new licence header.

Done.

      static constexpr size_t _Size = static_cast<size_t>(last_member) + 1;
Names that begin with an underscore followed by an uppercase letter are reserved.

Changed to s_size.

Can't this also be constexpr?

Done.

Might be worth adding begin() and end() as well?

Done. It might be useful to have it iterate over std::pair<T, V> like std::map, but that's a bit more complicated to implement (and not always useful).

Seems safer to value-initialize the array in case V is e.g. const char*

In my original use case, the user always needed to supply all values and there wasn't any no-arg constructor. But now that there's a no-arg constructor, this makes sense to add. Done.

// Template for checking if Types is count occurences of T.

typo: occurrence

Changed to "occurrences". The plural seems correct to me.

Why was this renamed?

I did a find-and-replace for LOG_TYPE to LogType, which also matched log_types and changed it to LogTypes. Fixed.

leoetlino

Reviewed 48 of 48 files at r3, 46 of 46 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Pokechu22)

Pokechu22 force-pushed the log-enum-class branch 2 times, most recently from 1696ad7 to 3e7f08a Compare October 21, 2021 19:34

Pokechu22 commented Oct 21, 2021

View reviewed changes

BhaaLseN reviewed Oct 22, 2021

View reviewed changes

Pokechu22 force-pushed the log-enum-class branch from 3e7f08a to 8c48930 Compare October 23, 2021 00:56

Pokechu22 force-pushed the log-enum-class branch from 8c48930 to 882ee96 Compare October 23, 2021 23:16

leoetlino requested changes Oct 24, 2021

View reviewed changes

Pokechu22 force-pushed the log-enum-class branch from 882ee96 to 776fdfb Compare October 24, 2021 18:45

Pokechu22 added 2 commits October 24, 2021 11:48

Create EnumMap

ba10781

Convert LOG_TYPE and LOG_LEVELS to enum class

04d8cdf

Pokechu22 force-pushed the log-enum-class branch from 776fdfb to 04d8cdf Compare October 24, 2021 18:48

leoetlino approved these changes Oct 24, 2021

View reviewed changes

leoetlino merged commit 85bbc0d into dolphin-emu:master Oct 24, 2021
9 of 10 checks passed

Convert LOG_TYPE and LOG_LEVELS to enum class #10182

Convert LOG_TYPE and LOG_LEVELS to enum class #10182

Pokechu22 commented Oct 21, 2021

Pokechu22 left a comment

Pokechu22 Oct 21, 2021

BhaaLseN Oct 22, 2021

Pokechu22 Oct 23, 2021

Pokechu22 Oct 21, 2021

Pokechu22 Oct 21, 2021

BhaaLseN Oct 22, 2021

Pokechu22 Oct 23, 2021

Pokechu22 Oct 21, 2021

BhaaLseN Oct 22, 2021

Pokechu22 Oct 23, 2021

Pokechu22 Oct 21, 2021

BhaaLseN Oct 22, 2021

Pokechu22 Oct 23, 2021

BhaaLseN Oct 22, 2021

BhaaLseN Oct 22, 2021

BhaaLseN Oct 22, 2021

Pokechu22 Oct 23, 2021

leoetlino commented Oct 23, 2021

leoetlino left a comment

Pokechu22 commented Oct 24, 2021

leoetlino left a comment

		GENERIC_LOG_FMT(Common::Log::LogType::VIDEO, static_cast<Common::Log::LogLevel>(level), "{}",
		real_msg);

Convert LOG_TYPE and LOG_LEVELS to enum class #10182

Convert LOG_TYPE and LOG_LEVELS to enum class #10182

Conversation

Pokechu22 commented Oct 21, 2021

Pokechu22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoetlino commented Oct 23, 2021

leoetlino left a comment

Choose a reason for hiding this comment

Pokechu22 commented Oct 24, 2021

leoetlino left a comment

Choose a reason for hiding this comment