erichkeane added inline comments.
================
Comment at: clang/lib/AST/Interp/Disasm.cpp:26
inline std::enable_if_t<!std::is_pointer<T>::value, T> ReadArg(Program &P,
- CodePtr OpPC) {
+ CodePtr &OpPC) {
return OpPC.read<T>();
----------------
tbaeder wrote:
> erichkeane wrote:
> > Are you sure this isn't intentional that this is passed by value? With a
> > name like CodePtr, it certainly SOUNDS like it means to be passed by value.
> `CodePtr::read()` advances the pointer that `CodePtr` wraps , so copying just
> to call `read()` on it makes little sense. E.g. in
> https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Interp/Interp.cpp#L401-L412,
> `ReadArg` must advance the `CodePtr`, otherwise two consecutive `ReadArg`
> calls will read the same thing.
my point is; With a struct this small, the 'copy' is cheaper than a
pass-by-ref. However, if the pass-by-value causes the ReadArg to read the same
thing, why was this not a problem before? More importantly, what in this patch
makes this change necessary?
================
Comment at: clang/test/AST/Interp/literals.cpp:14
+constexpr bool getTrue() { return true; }
+constexpr bool getFalse() { return false; }
----------------
tbaeder wrote:
> erichkeane wrote:
> > can you also do a 'getNullptr'? Also, a static-assert for all 3 of the
> > functions as well.
> I can do a `getNull()`, but I can't call any of the functions yet, function
> calls aren't implemented yet.
ah, thanks for that clarificaiton. When doing the function-calls, please see if
you can add that here as well.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131942/new/
https://reviews.llvm.org/D131942
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
