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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits