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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I didn't have the chance to complete my review (this is a pretty massive change), but here are some in-progress comments.



================
Comment at: clang/include/clang/Interpreter/Interpreter.h:105
 
+  Parser &getParser() const;
+
----------------
const mismatch here -- should be an overload set where the const member function returns a const reference and the non-const member function returns the non-const reference. (This is true in general, I see we've got other such functions that are const-qualified but returning non-const references.)

(Not critical to fix, but would be a nice follow-up NFC to correct these sorts of things; retro-fitting const correctness is hard, so we should strive for const correct by default for new code whenever possible.)


================
Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:1-3
+//===--- __clang_interpreter_runtime_printvalue.h - Incremental Compiation and
+// Execution---*- C++
+//-*-===//
----------------
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 at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:28
+
+// We should include it somewhere instead of duplicating it...
+#if __has_attribute(visibility) &&                                             \
----------------



================
Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:33
+#if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS)
+#define REPL_EXTERNAL_VISIBILITY __attribute__((visibility("default")))
+#else
----------------
You should use reserved identifiers where possible so as not to conflict with user-defined macros.


================
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);
+
----------------
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++?


================
Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:92
+
+namespace repl_runtime_detail {
+
----------------



================
Comment at: clang/lib/Headers/__clang_interpreter_runtime_printvalue.h:96
+// standards.
+template <typename... T> struct repl_make_void { typedef void type; };
+
----------------
(and so forth in this header -- basically, every identifier should be in the reserved namespace unless it's the public API being exposed.)


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:434
+
+llvm::Expected<llvm::orc::ExecutorAddr> Interpreter::CompileDecl(Decl *D) {
+  assert(D && "The Decl being compiled can't be null");
----------------
Any way to make this take a `const Decl *` instead?


================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:108
+  const DeclContext *DC = D->getDeclContext();
+  if (const NamespaceDecl *NS = dyn_cast<NamespaceDecl>(DC)) {
+    while (NS && NS->isInline()) {
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:111
+      // Ignore inline namespace;
+      NS = dyn_cast_or_null<NamespaceDecl>(NS->getDeclContext());
+    }
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:117-122
+  if (const auto *TD = dyn_cast<TagDecl>(DC)) {
+    return CreateNestedNameSpecifier(Ctx, TD, FullyQualify);
+  }
+  if (const TypedefNameDecl *TDD = dyn_cast<TypedefNameDecl>(DC)) {
+    return CreateNestedNameSpecifier(Ctx, TDD, FullyQualify);
+  }
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:133-136
+  const NamedDecl *Outer =
+      llvm::dyn_cast_or_null<NamedDecl>(D->getDeclContext());
+  const NamespaceDecl *OuterNs =
+      llvm::dyn_cast_or_null<NamespaceDecl>(D->getDeclContext());
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:139-140
+
+    if (const CXXRecordDecl *CXXD =
+            llvm::dyn_cast<CXXRecordDecl>(D->getDeclContext())) {
+
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:157-158
+          D = *(clTempl->spec_begin());
+          Outer = llvm::dyn_cast<NamedDecl>(D);
+          OuterNs = llvm::dyn_cast<NamespaceDecl>(D);
+        }
----------------



================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:163-168
+    if (OuterNs) {
+      return CreateNestedNameSpecifier(Ctx, OuterNs);
+    }
+    if (const auto *TD = llvm::dyn_cast<TagDecl>(Outer)) {
+      return CreateNestedNameSpecifier(Ctx, TD, FullyQualified);
+    }
----------------
I'll stop commenting on this sort of thing; you should apply this sort of cleanup to the whole patch.


================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:186-189
+    if (const auto *TT = llvm::dyn_cast_or_null<TagType>(TypePtr))
+      D = TT->getDecl();
+    else
+      D = TypePtr->getAsCXXRecordDecl();
----------------
So the first case is for handling enumerations and otherwise we expect a structure or a union? Similar question below as well.


================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:239
+  TemplateDecl *TD = Name.getAsTemplateDecl();
+  QualifiedTemplateName *qtname = Name.getAsQualifiedTemplateName();
+
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146809/new/

https://reviews.llvm.org/D146809