[PATCH] D146809: [clang-repl] Implement Value pretty printing

[Aaron Ballman via Phabricator via cfe-commits]

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