aaron.ballman added inline comments.
================ Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:44 +template <class T, class = typename std::enable_if_t<!std::is_pointer_v<T>>> +inline std::string PrintValueRuntime(const T &Val) { + return "{not representable}"; ---------------- Need to use a reserved identifier for everything so you don't conflict with user-defined macros. ================ Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:79 + +template <typename T, typename = void> struct is_iterable : std::false_type {}; + ---------------- Still need to make sure you're using reserved identifiers for these (should make a pass through the file and get all of the identifiers). ================ Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:81 + +// this gets used only when we can call std::begin() and std::end() on that type +template <typename T> ---------------- ================ Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:1-3 +//===--- __clang_interpreter_runtime_printvalue.h - Incremental Compiation and +// Execution---*- C++ +//-*-===// ---------------- aaron.ballman wrote: > er, not certain the best way to repair this, but wrapping the comment isn't > it. Maybe drop the "Incremental Compilation and Execution" bit? Comment can be re-flowed so this no longer wraps. ================ Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:50 +// Below overloads are all defined in the library itself. +REPL_EXTERNAL_VISIBILITY std::string PrintValueRuntime(const void *Ptr); + ---------------- aaron.ballman wrote: > I wonder if it makes some degree of sense to use macros to declare these, so > that it's easier to see there's a consistent pattern and which types are > supported. e.g., > ``` > #define __DECL_PRINT_VALUE_RUNTIME(type) __REPL_EXTERNAL_VISIBILITY > std::string PrintValueRuntime(const type *__Ptr) > > __DECL_PRINT_VALUE_RUNTIME(void); > __DECL_PRINT_VALUE_RUNTIME(void *); > __DECL_PRINT_VALUE_RUNTIME(bool); > ... > > #undef __DECL_PRINT_VALUE_RUNTIME > ``` > I also wonder: should this header have a C interface so it can be used from a > C context, or is the repl context in which this is included only ever C++? Still wondering about the C interface question. ================ Comment at: clang/lib/Interpreter/Value.cpp:272 + void Value::print(llvm::raw_ostream &Out) const { assert(OpaqueType != nullptr && "Can't print default Value"); ---------------- junaire wrote: > v.g.vassilev wrote: > > We should add some documentation for users how to write a pretty-printer > > for a custom class. > > > > We do not seem to support the multiple inheritance case where one of the > > base classes has a pretty-printer and the other does not. See the > > explanation in Cling. It is okay to not have it at the moment but we should > > include a FIXME here saying that we do not yet support it. > Where should I put the doc? Value.cpp is the implementation and it's unlikely > for regular users to read it. I'd expect we'd be updating `clang/docs/ClangRepl.rst` for this sort of thing. ================ Comment at: clang/lib/Interpreter/ValuePrinter.cpp:89-90 + + if (QT->isReferenceType()) + SS << " &"; + ---------------- This doesn't distinguish between lvalue and rvalue references, which seems like a mistake. ================ Comment at: clang/lib/Interpreter/ValuePrinter.cpp:164 + +// TODO: Encodings. +static std::string PrintOneChar(char Val) { ---------------- Not just encodings, but also: how do you handle unprintable characters (like control characters)? ================ Comment at: clang/lib/Interpreter/ValuePrinter.cpp:543 + default: + llvm_unreachable("Unknown Builtintype kind"); + } ---------------- This seems rather reachable, no? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146809/new/ https://reviews.llvm.org/D146809 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits