[Lldb-commits] [PATCH] D117490: [lldb] Make logging machinery type-safe

[Pavel Labath via Phabricator via lldb-commits]

labath marked 7 inline comments as done.
labath added a comment.

In D117490#3261743 <https://reviews.llvm.org/D117490#3261743>, @shafik wrote:

> This is a nice refactor, I am curious was there a motivating bug or issue for this change or just refactoring?

Well.. my motivation was D117382 <https://reviews.llvm.org/D117382>. I don't know what motivated him to do the big refactor (I know he was running out of log category bits, but I think that could be solved without it), but one of the things both of us are trying to solve is to make sure one cannot mismatch log category flags and log getter function (we have some code doing that right now).



================
Comment at: lldb/include/lldb/Utility/Log.h:57
+  /// };
+  using MaskType = uint32_t;
+
----------------
clayborg wrote:
> JDevlieghere wrote:
> > Didn't this all start with Greg wanting to make this a `uint64_t`?
> yes, uint64_t please!
I wanted to keep that for a separate patch. I was preparing for that by introducing the new type(def). Though, since this is now really just about changing this single identifier, I guess I might as well do it immediately.


================
Comment at: lldb/include/lldb/Utility/Logging.h:19-50
+  API = Log::ChannelFlag<0>,
+  AST = Log::ChannelFlag<1>,
+  Breakpoints = Log::ChannelFlag<2>,
+  Commands = Log::ChannelFlag<3>,
+  Communication = Log::ChannelFlag<4>,
+  Connection = Log::ChannelFlag<5>,
+  DataFormatters = Log::ChannelFlag<6>,
----------------
JDevlieghere wrote:
> I would put this into a `.def` file and have it generate both this list as well as the one with the macros below. The last entry is annoying. We could either omit it from the list but then you risk someone updating the def file but not this. I don't think there's a way to do this automatically? Maybe just an additional define? 
I'm planning to delete the macros more-or-less immediately after this patch goes in. I didn't want to do that in the same patch as it would make hide the "interesting" changes between thousands of machanical edits.

So, I wouldn't want to create a def file just because of that. I can imagine creating both this list and the Log::Category definition with a def file, though I'm not sure if it's really worth it.

As for `LLVM_MARK_AS_BITMASK_ENUM`, if I was going with a def file, I'd probably make this `LLVM_MARK_AS_BITMASK_ENUM(std::numeric_limits<MaskType>::max())` (it does not have to refer to an actual constant). The only thing we'd "lose" that way is some assertion failures if someone passes an out-of-range constant in some way...


================
Comment at: lldb/unittests/Utility/LogTest.cpp:34-38
+static Log::Channel test_channel(test_categories, TestChannel::FOO);
+
+namespace lldb_private {
+template <> Log::Channel &LogChannelFor<TestChannel>() { return test_channel; }
+} // namespace lldb_private
----------------
clayborg wrote:
> Should we make a macro for this in Log.h? Something that would define the log channel global variable, and make the template for us?
> 
> ```
> LLDB_CREATE_LOG(test_categories, TestChannel, TestChannel::FOO);
> ```
> which would expand to the above code?
I thought about this for a long time. The macro adds some convenience, but it does not seem right to me. The macro would hide the fact that there are two objects being declared here, with different storage classes. The storage class part is important, because if one tried to put this declaration in a header, it could almost work, except that the `LogChannelFor` function would return a different (static) object in each compile unit (which would be an odr violation, of course).

And I am actually thinking about moving this stuff to a header, since ideally we'd want to compiler to inline the `LogChannelFor` function call. That would mean the user would need to forward-declare the Channel object in the header, and define it in the cpp file, and it would be weird to define an object declared by a macro.

I think that moving this to a header would also reduce the boilerplate slightly, since one wouldn't need to forward-declare *and* define the `LogChannelFor` specialization.

At the end of the day, I don't think this matters much, as we don't create logging channels very often.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117490/new/

https://reviews.llvm.org/D117490