erichkeane accepted this revision.
erichkeane added inline comments.
This revision is now accepted and ready to land.
================
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:
> > 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?
> Nothing in this patch, if you e.g. take this code:
> ```
> constexpr int func() { return 5; }
> ```
> and run it with `clang++ -std=c++20 -fexperimental-new-constant-interpreter`,
> the constant interpreter runs into an assertion while evaluating the bytecode
> for that function. Switching `ReadArg` to take the codeptr by reference fixes
> that. So I could probably postpone the change to a later patch but it's one
> of the first problems I ran into while working on the codebase.
I see, thanks for the explanation. I think it is fine now, it just wasn't
clear that this was covered by a test or necessary.
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
