================
@@ -0,0 +1,93 @@
+//===-- Telemetry.h ----------------------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_CORE_TELEMETRY_H
+#define LLDB_CORE_TELEMETRY_H
+
+#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Telemetry/Telemetry.h"
+#include <chrono>
+#include <ctime>
+#include <memory>
+#include <optional>
+#include <string>
+#include <unordered_map>
+
+namespace lldb_private {
+
+struct LldbEntryKind : public ::llvm::telemetry::EntryKind {
+ static const llvm::telemetry::KindType BaseInfo = 0b11000;
+};
+
+/// Defines a convenient type for timestamp of various events.
+/// This is used by the EventStats below.
+using SteadyTimePoint = std::chrono::time_point<std::chrono::steady_clock,
+ std::chrono::nanoseconds>;
+
+/// Various time (and possibly memory) statistics of an event.
+struct EventStats {
+ // REQUIRED: Start time of an event
+ SteadyTimePoint start;
----------------
labath wrote:
I think what caught Jonas's eye (though he probably doesn't know it) is that
this class was very protobuf-y. Let me try to unpack that. We don't have the
(understandable if you know the background, but strange if you don't) situation
of fields that are *optional* at the transport layer but *required* at the
application layer elsewhere. And I don't think we need that here either because
the transport layer is part of the downstream/vendor implementation, so we can
just use regular c++ conventions for the application layer (if a type is
std::optional, it's optional; if it's not, it's not).
That's one part of the issue. The second part is question whether the
"required-ness" of a field can be enforced syntactically (through a constructor
which intializes it). That's usually a nice property though it doesn't work in
all situations, and I don't think it's that useful for objects which are plain
data carriers (no internal logic). It also only works if *every* constructor
initializes it, so the fact that you've added a constructor which does that,
but then also kept the default one which does not (well, it initializes it, but
to zero), doesn't really help there.
I'm saying this because this overload set is basically the same as what you'd
get if you specified no explicit constructors -- you could still do the same
thing, just with curly braces (`EventStats{}` default initializes everything,
`EventStats{start}` initializes the first member, `EventStats{start, end}`
initializes both). So, if this is the behavior you want, you can just delete
all of them. If you want to enforce the requiredness (which might be nice, but
I don't think it's necessary -- Jonas may disagree), then you should delete
the default constructor -- but then you also need to use these classes
differently.
https://github.com/llvm/llvm-project/pull/119716
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
