sammccall added a comment.

This seems correct but it also seems like it makes setting all the args of a 
CallExpr into a quadratic operation.
e.g. ConvertArgumentForCall and BuildResolvedCallExpr in SemaExpr seem to do 
this (not just missing/changed args) and are probably on common code paths.
It's hard to see how to fix this without touching the API or being fiddly with 
the dependence bits (e.g. setArg only needs to rescan all args if we're 
removing at least one dep bit vs the old value, which should be rare).

Also this function just calls through to the (mutable) getArgs() which returns 
a mutable Expr**. This allows mutating args, bypassing setArg(), and ISTM there 
are cases where this really happens e.g. in TreeTransform::TransformCallExpr.
Again, it seems like changing the API might be the answer here: we can separate 
out the getArgs() which only need read access from those that are doing 
something tricky.

-

A tempting API would be something like getMutableArgs() -> some smart object 
that exposes the mutable arg array and also recalculates dependence when 
destroyed.
I guess the way to make this ergonomic is to actually inherit from 
MutableArrayRef, like:

  class CallExpr::MutableArgs : public MutableArrayRef<Expr> {
    CallExpr *Parent;
  public:
    // Note that "slicing" copy to MutableArrayRef is still allowed.
    MutableArgs(const MutableArgs&) = delete;
    MutableArgs &operator=(const MutableArgs&) = delete;
    MutableArgs(MutableArgs&&) = delete;
    MutableArgs &operator=(MutableArgs&&) = delete;
  
    ~MutableArgs() { Parent->recomputeDependence(); }
  };

I'm not sure if you see this as overengineering.
I think I can also live with adding explicit recomputeDependence() calls in the 
right places, and slapping a warning on setArg() that it might be needed.
But i'm not sure that making setArg() implicitly recalculate is a happy 
in-between point. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104052

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to