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

[Mike K via Phabricator via flang-commits]

FruitClover 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:
----------------
jeanPerier wrote:
> 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_;
> }
>   
> }
> ```
> 
Thanks a lot for detailed feedback, suggested change looks much better, updated.


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

https://reviews.llvm.org/D123520