junaire added inline comments.
================ Comment at: clang/include/clang/Interpreter/Interpreter.h:96 + + size_t getEffectivePTUSize() const; + ---------------- aaron.ballman wrote: > It looks like this can be private? > > Also, just a note (not something you have to deal with in this review), the > local naming convention seems to have decided that if the function starts > with `get` then it's camel case and otherwise the name is pascal case. Fixed. ================ Comment at: clang/include/clang/Interpreter/Value.h:83 + Value() = default; + Value(void /*Interpreter*/ *In, void /*QualType*/ *Ty); + Value(const Value &RHS); ---------------- aaron.ballman wrote: > v.g.vassilev wrote: > > junaire wrote: > > > aaron.ballman wrote: > > > > Why do these take `void *` instead of the expected type? > > > Yeah for the first parameter, we could just use `Interpreter*` but the > > > second one is an opaque type so I think we should keep it? > > See my previous comments on performance. We cannot include anything bulky > > in the header file. > I think I understand why the design is the way it is, but it still makes me > uneasy. The constructor takes a pointer to some bucket of bytes... no size > information, no type information, etc. Just "here's a random pointer". And > then later, we hope the user calls `setKind()` in a way that makes sense. > > We need it to be fast, but we also need it to be correct -- the type system > is the best tool for helping with that. Not really... The user doesn't need to call `setKind()` explicitly to construct a `Value`, the constructor will handle it automatically. See `ConvertQualTypeToKind` in `Value.cpp`. So if the pointer is just some garbage data, the constructor should fail before yielding out a valid instance. ================ Comment at: clang/lib/Interpreter/IncrementalParser.cpp:22 #include "clang/FrontendTool/Utils.h" +#include "clang/Interpreter/Interpreter.h" #include "clang/Parse/Parser.h" ---------------- aaron.ballman wrote: > v.g.vassilev wrote: > > This introduces a layering violation. Can we avoid it? > Does it? This file is in `clang/lib/Interpreter` so including other things > from `clang/Interpreter` would not be a layering violation normally. That's an old comment, already fixed and marked it as done :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141215/new/ https://reviews.llvm.org/D141215 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
