Refactor error_helper and around (#4686)

* Modernize the code: - Use universal references - Consistently forward the packs - Ensure we move things passed by value - Use C++17 constructs * Simplify parser_error: switch for variadic functions to variadic templates. Removes some layers of indirection. * Greatly simplify error_helper. Unbreak HasToString while there. * Ensure we do not do extra copy of std::string during toString * Forward-declare big_int * Consolidate all => string conversions in a single place: - prefer dbprint() method for operator<<(ostream&) when present - otherwise, fallback to toString(), if present * Simplify bug_helper more, hide stuff into dedicated namespace to reduce amount of visible overload variants * Unbreak for Boost < 1.84 * Simplify * Silence broken gcc warning * Try to unbreak bazel. Again * FIXME was clarified
p4lang · Jun 2, 2024 · 4557b06 · 4557b06
1 parent b94e80c
commit 4557b06
Show file tree

Hide file tree

Showing 23 changed files with 305 additions and 350 deletions.
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -54,6 +54,7 @@ cc_library(
         "@boost//:multiprecision",
         "@com_google_absl//absl/numeric:bits",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/strings:str_format",
         "@com_google_googletest//:gtest",
     ],
 )

diff --git a/backends/p4tools/common/core/z3_solver.cpp b/backends/p4tools/common/core/z3_solver.cpp
@@ -13,6 +13,7 @@
 
 #include <boost/multiprecision/cpp_int.hpp>
 
+#include "absl/strings/str_format.h"
 #include "ir/ir.h"
 #include "ir/irutils.h"
 #include "ir/json_loader.h"  // IWYU pragma: keep
@@ -31,18 +32,9 @@ namespace P4Tools {
 const char *toString(const z3::expr &e) { return Z3_ast_to_string(e.ctx(), e); }
 
 #ifndef NDEBUG
-template <typename... Args>
-std::string stringFormat(const char *format, Args... args) {
-    size_t size = snprintf(nullptr, 0, format, args...) + 1;
-    BUG_CHECK(size > 0, "Z3Solver: error during formatting.");
-    std::unique_ptr<char[]> buf(new char[size]);
-    snprintf(buf.get(), size, format, args...);
-    return {buf.get(), buf.get() + size - 1};
-}
-
-#define Z3_LOG(FORMAT, ...)                                                                       \
-    LOG1(stringFormat("Z3Solver:%s() in %s, line %i: " FORMAT "\n", __func__, __FILE__, __LINE__, \
-                      __VA_ARGS__))
+#define Z3_LOG(FORMAT, ...)                                                                \
+    LOG1(absl::StrFormat("Z3Solver:%s() in %s, line %i: " FORMAT "\n", __func__, __FILE__, \
+                         __LINE__, __VA_ARGS__))
 
 /// Converts a Z3 model to a string.
 const char *toString(z3::model m) { return Z3_model_to_string(m.ctx(), m); }

diff --git a/backends/p4tools/modules/testgen/lib/exceptions.h b/backends/p4tools/modules/testgen/lib/exceptions.h
@@ -1,6 +1,7 @@
 #ifndef BACKENDS_P4TOOLS_MODULES_TESTGEN_LIB_EXCEPTIONS_H_
 #define BACKENDS_P4TOOLS_MODULES_TESTGEN_LIB_EXCEPTIONS_H_
 
+#include "absl/strings/str_cat.h"
 #include "lib/exceptions.h"
 
 namespace P4Tools::P4Testgen {
@@ -9,21 +10,21 @@ namespace P4Tools::P4Testgen {
 /// Paths with this unimplemented feature should be skipped
 class TestgenUnimplemented final : public Util::P4CExceptionBase {
  public:
-    template <typename... T>
-    explicit TestgenUnimplemented(const char *format, T... args)
-        : P4CExceptionBase(format, args...) {
+    template <typename... Args>
+    explicit TestgenUnimplemented(const char *format, Args &&...args)
+        : P4CExceptionBase(format, std::forward<Args>(args)...) {
         // Check if output is redirected and if so, then don't color text so that
         // escape characters are not present
-        message = cstring(Util::cerr_colorize(Util::ANSI_BLUE)) + "Not yet implemented" +
-                  Util::cerr_clear_colors() + ":\n" + message;
+        message = absl::StrCat(Util::cerr_colorize(Util::ANSI_BLUE), "Not yet implemented",
+                               Util::cerr_clear_colors(), ":\n", message);
     }
 
-    template <typename... T>
-    TestgenUnimplemented(int line, const char *file, const char *format, T... args)
-        : P4CExceptionBase(format, args...) {
-        message = cstring("In file: ") + file + ":" + Util::toString(line) + "\n" +
-                  Util::cerr_colorize(Util::ANSI_BLUE) + "Unimplemented compiler support" +
-                  Util::cerr_clear_colors() + ": " + message;
+    template <typename... Args>
+    TestgenUnimplemented(int line, const char *file, const char *format, Args &&...args)
+        : P4CExceptionBase(format, std::forward<Args>(args)...) {
+        message = absl::StrCat(
+            "In file: ", file, ":", line, "\n", Util::cerr_colorize(Util::ANSI_BLUE),
+            "Unimplemented compiler support", Util::cerr_clear_colors(), ": ", message);
     }
 };
 

diff --git a/frontends/p4/typeChecking/typeChecker.h b/frontends/p4/typeChecking/typeChecker.h
@@ -63,9 +63,9 @@ class TypeChecking : public PassManager {
                  bool updateExpressions = false);
 };
 
-template <typename... T>
-void typeError(const char *format, T... args) {
-    ::error(ErrorType::ERR_TYPE_ERROR, format, args...);
+template <typename... Args>
+void typeError(const char *format, Args &&...args) {
+    ::error(ErrorType::ERR_TYPE_ERROR, format, std::forward<Args>(args)...);
 }
 /// True if the type contains any varbit or header_union subtypes
 bool hasVarbitsOrUnions(const TypeMap *typeMap, const IR::Type *type);

diff --git a/ir/expression.def b/ir/expression.def
@@ -46,7 +46,8 @@ abstract Operation_Binary : Operation {
             type = left->type; }
     toString {
         std::stringstream tmp;
-        tmp << DBPrint::Prec_Low << *this;
+        tmp << DBPrint::Prec_Low;
+        dbprint(tmp);
         return tmp.str();
     }
 }

diff --git a/ir/visitor.h b/ir/visitor.h
@@ -294,20 +294,14 @@ class Visitor {
     /// Static version of the above function, which can be called
     /// even if not directly in a visitor
     static bool warning_enabled(const Visitor *visitor, int warning_kind);
-    template <
-        class T,
-        typename = typename std::enable_if<std::is_base_of<Util::IHasSourceInfo, T>::value>::type,
-        class... Args>
-    void warn(const int kind, const char *format, const T *node, Args... args) {
+    template <class T, typename = std::enable_if_t<Util::has_SourceInfo_v<T>>, class... Args>
+    void warn(const int kind, const char *format, const T *node, Args &&...args) {
         if (warning_enabled(kind)) ::warning(kind, format, node, std::forward<Args>(args)...);
     }
 
     /// The const ref variant of the above
-    template <
-        class T,
-        typename = typename std::enable_if<std::is_base_of<Util::IHasSourceInfo, T>::value>::type,
-        class... Args>
-    void warn(const int kind, const char *format, const T &node, Args... args) {
+    template <class T, typename = std::enable_if_t<Util::has_SourceInfo_v<T>>, class... Args>
+    void warn(const int kind, const char *format, const T &node, Args &&...args) {
         if (warning_enabled(kind)) ::warning(kind, format, node, std::forward<Args>(args)...);
     }
 

diff --git a/lib/backtrace_exception.h b/lib/backtrace_exception.h
@@ -37,7 +37,7 @@ class backtrace_exception : public E {
 
  public:
     template <class... Args>
-    explicit backtrace_exception(Args... args) : E(std::forward<Args>(args)...) {
+    explicit backtrace_exception(Args &&...args) : E(std::forward<Args>(args)...) {
 #if HAVE_EXECINFO_H
         backtrace_size = backtrace(backtrace_buffer, buffer_size);
 #else

diff --git a/lib/big_int_fwd.h b/lib/big_int_fwd.h
@@ -0,0 +1,24 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+#ifndef LIB_BIG_INT_FWD_H_
+#define LIB_BIG_INT_FWD_H_
+
+#include <memory>
+
+#include <boost/multiprecision/fwd.hpp>
+
+using big_int = boost::multiprecision::cpp_int;
+
+#endif /* LIB_BIG_INT_FWD_H_ */
diff --git a/lib/bug_helper.h b/lib/bug_helper.h
@@ -17,51 +17,23 @@ limitations under the License.
 #ifndef LIB_BUG_HELPER_H_
 #define LIB_BUG_HELPER_H_
 
+#include <sstream>
 #include <string>
 #include <string_view>
+#include <utility>
 
 #include <boost/format.hpp>
 
 #include "absl/strings/str_cat.h"
-#include "lib/cstring.h"
-#include "lib/source_file.h"
+#include "cstring.h"
+#include "source_file.h"
+#include "stringify.h"
 
-template <class... Args>
-std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
-                       const char *t, Args &&...args);
-
-template <class... Args>
-std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
-                       const cstring &t, Args &&...args);
-
-template <class... Args>
-std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
-                       const Util::SourceInfo &info, Args &&...args);
-
-template <typename T, class... Args>
-auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T &t,
-                Args &&...args)
-    -> std::enable_if_t<std::is_base_of_v<Util::IHasSourceInfo, T>, std::string>;
-
-template <typename T, class... Args>
-auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T &t,
-                Args &&...args)
-    -> std::enable_if_t<!std::is_base_of_v<Util::IHasSourceInfo, T>, std::string>;
-
-template <typename T, class... Args>
-auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T *t,
-                Args &&...args)
-    -> std::enable_if_t<std::is_base_of_v<Util::IHasSourceInfo, T>, std::string>;
-
-template <typename T, class... Args>
-auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T *t,
-                Args &&...args)
-    -> std::enable_if_t<!std::is_base_of_v<Util::IHasSourceInfo, T>, std::string>;
-
-// actual implementations
 namespace detail {
-static inline auto getPositionTail(const Util::SourceInfo &info, std::string_view position,
-                                   std::string_view tail) {
+
+static inline std::pair<std::string_view, std::string> getPositionTail(const Util::SourceInfo &info,
+                                                                       std::string_view position,
+                                                                       std::string_view tail) {
     std::string_view posString = info.toPositionString().string_view();
     std::string outTail(tail);
     if (position.empty()) {
@@ -74,38 +46,34 @@ static inline auto getPositionTail(const Util::SourceInfo &info, std::string_vie
 
     return std::pair(position, outTail);
 }
-}  // namespace detail
 
-static inline std::string bug_helper(boost::format &f, std::string_view position,
-                                     std::string_view tail) {
-    return absl::StrCat(position, position.empty() ? "" : ": ", boost::str(f), "\n", tail);
-}
+template <typename T>
+std::pair<std::string_view, std::string> maybeAddSourceInfo(const T &t, std::string_view position,
+                                                            std::string_view tail) {
+    if constexpr (Util::has_SourceInfo_v<T>)
+        return getPositionTail(t.getSourceInfo(), position, tail);
 
-template <class... Args>
-std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
-                       const char *t, Args &&...args) {
-    return bug_helper(f % t, position, tail, std::forward<Args>(args)...);
+    (void)position;
+    (void)tail;
+    return {"", ""};
 }
 
-template <class... Args>
-std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
-                       const cstring &t, Args &&...args) {
-    return bug_helper(f % t.string_view(), position, tail, std::forward<Args>(args)...);
+static inline std::string bug_helper(boost::format &f, std::string_view position,
+                                     std::string_view tail) {
+    return absl::StrCat(position, position.empty() ? "" : ": ", boost::str(f), "\n", tail);
 }
 
 template <typename T, class... Args>
 auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T *t,
-                Args &&...args)
-    -> std::enable_if_t<!std::is_base_of_v<Util::IHasSourceInfo, T>, std::string> {
-    std::stringstream str;
-    str << t;
-    return bug_helper(f % str.str(), position, tail, std::forward<Args>(args)...);
-}
+                Args &&...args);
 
 template <typename T, class... Args>
 auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T &t,
-                Args &&...args)
-    -> std::enable_if_t<!std::is_base_of_v<Util::IHasSourceInfo, T>, std::string> {
+                Args &&...args) -> std::enable_if_t<!std::is_pointer_v<T>, std::string>;
+
+template <class... Args>
+std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
+                       const char *t, Args &&...args) {
     return bug_helper(f % t, position, tail, std::forward<Args>(args)...);
 }
 
@@ -118,25 +86,38 @@ std::string bug_helper(boost::format &f, std::string_view position, std::string_
 
 template <typename T, class... Args>
 auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T *t,
-                Args &&...args)
-    -> std::enable_if_t<std::is_base_of_v<Util::IHasSourceInfo, T>, std::string> {
+                Args &&...args) {
     if (t == nullptr) return bug_helper(f, position, tail, std::forward<Args>(args)...);
 
-    auto [outPos, outTail] = detail::getPositionTail(t->getSourceInfo(), position, tail);
+    auto [outPos, outTail] = maybeAddSourceInfo(*t, position, tail);
+    // We define operator<< for all T's that have either dbprint() or toString()
+    // methods (in stringify.h).  Note, however, that these overloads are
+    // provided in the global namespace, not namespace of the T itself and
+    // therefore cannot be found via ADL. As a result, name lookup for the `str
+    // << t` below succeeds as it is encloding namespace for `detail`. However,
+    // `f % t` will fail as operator<< will be called from some
+    // `boost::io::detail` namespace for which global namespace is not a direct
+    // enclosing one and therefore the lookup for operator<< will fail.
     std::stringstream str;
     str << t;
     return bug_helper(f % str.str(), outPos, outTail, std::forward<Args>(args)...);
 }
 
 template <typename T, class... Args>
 auto bug_helper(boost::format &f, std::string_view position, std::string_view tail, const T &t,
-                Args &&...args)
-    -> std::enable_if_t<std::is_base_of_v<Util::IHasSourceInfo, T>, std::string> {
-    auto [outPos, outTail] = detail::getPositionTail(t.getSourceInfo(), position, tail);
-
+                Args &&...args) -> std::enable_if_t<!std::is_pointer_v<T>, std::string> {
+    auto [outPos, outTail] = maybeAddSourceInfo(t, position, tail);
     std::stringstream str;
     str << t;
     return bug_helper(f % str.str(), outPos, outTail, std::forward<Args>(args)...);
 }
+}  // namespace detail
+
+// Most direct invocations of bug_helper usually only reduce arguments
+template <class... Args>
+std::string bug_helper(boost::format &f, std::string_view position, std::string_view tail,
+                       Args &&...args) {
+    return detail::bug_helper(f, position, tail, std::forward<Args>(args)...);
+}
 
 #endif /* LIB_BUG_HELPER_H_ */