[flang-commits] [PATCH] D123237: [flang] Fix LBOUND() folding for constant arrays

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for working on a fix. I think there is a second issue that would be worth fixing while you are working on this. The PackageConstant looks good to me, but I would not expose it in a tools.h header when it is only really needed here. Please see my comment on your UBOUND patch. I think there is an opportunity to centralize more in `GetConstantArrayHelper` and make its interface even clearer.



================
Comment at: flang/include/flang/Evaluate/tools.h:1044
+template <typename T>
+Constant<T> PackageConstant(
+    const ConstantSubscripts &&bounds, bool asScalar = false) {
----------------
I think `PackageConstant` is very specific to the LBOUND/UBOUND folding usage. I am not sure it is worth exposing it in this widely included header.

Looking at `GetConstantArrayLboundHelper`, it is only ever used in folding, so maybe `PackageConstant` could be moved inside it, or next to it if that brings too many templates.


================
Comment at: flang/lib/Evaluate/fold-integer.cpp:40
     // Strip off the parentheses
     return GetLbound(x.left());
   }
----------------
Since you are fixing LBOUND and UBOUND bugs, I think this part of the previous code is incorrect, as far as I can tell, the lower bounds of `(cst)` are ones, not the ones of cst.

e.g:
```
   integer, parameter :: a3(2:4,4:6) = 0
   print *, LBOUND((a3), 1)
end
```

Should print `1`, not `2`, and I think returning `GetLbound(x.left())` will cause it to return `2`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123237