[PATCH] D100989: [flang] Fix checking of argument passing for parameterized derived types

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld added inline comments.


================
Comment at: flang/lib/Evaluate/tools.cpp:1181
+// Used to check assignment statements and argument passing.  See 15.5.2.4(4)
+bool AreTypeParamCompatible(const semantics::DerivedTypeSpec &type1,
+    const semantics::DerivedTypeSpec &type2) {
----------------
klausler wrote:
> I'm not sure that this is behaviorally identical with AreKindCompatible(), which only compares KIND type parameters and ignores LEN type parameters.
It's not identical in that it checks both KIND and LEN type parameters.  My reading of the standard is that for objects of a parameterized derived type to be compatible, either in assignment statements or in argument passing, the constant values of the type parameters have to be equal.  That's true for both KIND and LEN type parameters.  For KIND parameters, the constant value is available at compile time.  In this case, the checking for the expressions being compile time constants is unnecessary, but it doesn't do any harm.  For LEN parameters, it's possible for the parameter to not have a constant expression available at compile time.  In this case, checking must be deferred to run time, and we shouldn't emit error messages at compile time.

There was only one call to `AreKindCompatible()`, which I replaced, so I deleted that function.


================
Comment at: flang/lib/Evaluate/tools.cpp:1185
+    if (semantics::MaybeIntExpr paramExpr1{param1.GetExplicit()}) {
+      if (evaluate::IsConstantExpr(*paramExpr1)) {
+        const semantics::ParamValue *param2{type2.FindParameter(name)};
----------------
klausler wrote:
> You don't need to check that the expressions are "constant expressions" in the Fortran standard's sense if you're going to be extracting their actual integer constant values later with ToInt64().  I know that you didn't write this (at least not for this patch), but while you're moving this code we might as well simplify it.
> 
> const auto param2{type2.FindParameter(name)};
> return !param2 || ToInt64(param1.GetExplicit()) == ToInt64(param2->GetExplicit();
> 
> The "evaluate::" namespace name isn't needed now that this code is in that namespace.
Thanks for the suggestion.  The code is much cleaner with these changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100989