[PATCH] D131942: [clang][Interp] Implement bool and nullptr literal expressions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked 3 inline comments as done.
tbaeder 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>();
----------------
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.


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

https://reviews.llvm.org/D131942