[flang-commits] [PATCH] D123520: [flang] Fix UBOUND() folding for constant arrays

[Jean Perier via Phabricator via flang-commits]

jeanPerier added inline comments.


================
Comment at: flang/lib/Evaluate/fold-integer.cpp:22
 // array that devolves to a type of Constant<T>
-class GetConstantArrayLboundHelper {
+template <typename HelperType> class GetConstantArrayBoundHelper {
 public:
----------------
I think the template makes this helper class interface a bit too complex for what it does. I am not sure it is worth making this a template parameter (templates are nice, but they have f18 compile time cost and f18 executable size costs, in this case, I do not think the template adds a lot of type safety).

What about doing having an interface like:

`static Expr<T> GetConstantArrayBound::GetLbound(const Expr<SomeType>&, std::optional<int> dim);`
and 
`static Expr<T> GetConstantArrayBound::GetUbound(const Expr<SomeType>&, std::optional<int> dim);`

with the implementation looking something like:

```
template <typename T>
Expr<T> PackageConstantBounds(
    const ConstantSubscripts &&bounds, bool asScalar = false) {
  // .... like  PackageConstant, but returns Expr<T>
}

class GetConstantArrayBound {
public:
   static Expr<T> GetConstantArrayBound::GetLbound(const Expr<SomeType>& array, std::optional<ConstantSubscript> dim) {
     return PackageConstantBounds<T>(GetConstantArrayBound(dim, /*getLbound=*/true)::Get(array), dim.has_value());
  }
  //... same for GetUbound
private:
    // Private ctor ...

    ConstantSubscripts Get(const Constant<T> &x) {
      if (getLbound) { 
        // Return the lower bound
        if (dim_) {
          return {x.lbounds().at(*dim_)};
        } else {
          return x.lbounds();
        }
      } else {
        return  x.ComputeUbounds(dim_);
      }
  }

  // Deal with the const Parentheses<T> &x. (return ones for lbouds, and the left operand shape for ubound).
  
  const std::optional<ConstantSubscript> dim_;
  const bool getLBound_;
}
  
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123520