[llvm] [llvm]Add a simple Telemetry framework (PR #102323)
Pavel Labath via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 13 02:06:23 PST 2024
================
@@ -0,0 +1,132 @@
+//===- llvm/Telemetry/Telemetry.h - Telemetry -------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file provides the basic framework for Telemetry
+/// Refer to its documentation at llvm/docs/Telemetry.rst for more details.
+//===---------------------------------------------------------------------===//
+
+#ifndef LLVM_TELEMETRY_TELEMETRY_H
+#define LLVM_TELEMETRY_TELEMETRY_H
+
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/JSON.h"
+#include <memory>
+#include <optional>
+#include <string>
+
+namespace llvm {
+namespace telemetry {
+
+class Serializer {
+public:
+ virtual llvm::Error init() = 0;
+ virtual void write(StringRef KeyName, bool Value) = 0;
+ virtual void write(StringRef KeyName, int Value) = 0;
+ virtual void write(StringRef KeyName, size_t Value) = 0;
----------------
labath wrote:
I think this isn't a combination choice of integer types as it can lead to platform-specific build errors. Right now, this is okay-ish, but if you later decide to e.g. add an `unsigned int` overload (e.g. because you have an unsigned int variable you want to print, and want to avoid ambiguous overload errors), then you'd break the 32bit build because on 32bit systems `size_t` *is* `unsigned int`. However, even without the extra overload you can run into the situation where code complies only on some platforms (e.g. `write(UINT64_C(1))` would build on (some?, depending on how they define these types) 64-bit systems but never on 32-bit ones)
I think it'd be better to choose either the fundamental types (`(un)?signed ((long?) long)? int`) or the fixed-width typedefs (`(u)?int(32|64)_t`) -- each have their tradeoffs(*) and then stick to those. Don't combine them, and definitely don't bring in the types with platform-specific widths.
(*) The problem with fundamental types is that there are more of them, and you also can't rely on their bitwidth in the implementation. The problem with fixed-width types is that you can you can still get into ambiguous situations if e.g. `uint64_t` is defined as `unsigned long`, but you have a (64-bit) variable of type `unsigned long long` (or vice versa).
(Assuming you don't want/need the ability to provide different implementations for different widths, it's also possible to provide just two overloads for maximum-width types (of both signednesses) and then use a (non-virtual) template function to dispatch to the correct implementation: https://godbolt.org/z/16cTva1fY)
https://github.com/llvm/llvm-project/pull/102323
More information about the llvm-commits
mailing list