[flang-commits] [PATCH] D79851: [Flang] Semantics for SELECT TYPE

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld requested changes to this revision.
PeteSteinfeld added a comment.
This revision now requires changes to proceed.

Thanks for working on this!

Let me know if you have any questions.



================
Comment at: flang/lib/Semantics/check-select-type.cpp:86
+
+  bool PerformChecksOnGuard(const parser::TypeGuardStmt::Guard &guard,
+      const evaluate::DynamicType &guardDynamicType) {
----------------
A more evocative name would be "PassesChecksOnGuard()"


================
Comment at: flang/lib/Semantics/check-select-type.cpp:93-94
+              if (const DeclTypeSpec * spec{typeSpec.declTypeSpec}) {
+                if (spec->category() == DeclTypeSpec::Character &&
+                    !guardDynamicType.IsAssumedLengthCharacter()) { // C1160
+                  context_.Say(parser::FindSourceLocation(typeSpec),
----------------
C1160 applies to LEN type parameters for both character and derived types.  It doesn't look like you're checking or testing for LEN parameters on derived types.  Can you please add a check and test?


================
Comment at: flang/lib/Semantics/check-select-type.cpp:118
+  }
+  bool ChecksOnDerivedTypeSpec(const semantics::DerivedTypeSpec &derived,
+      parser::CharBlock sourceLoc) const {
----------------
A more evocative name would be "PassesDerivedTypeChecks()".


================
Comment at: flang/lib/Semantics/check-select-type.cpp:122-123
+      context_.Say(sourceLoc,
+          "The type specification statement must not specify "
+          "a type with SEQUENCE attribute or a BIND attribute"_err_en_US);
+      return false;
----------------
It would be more consistent for the message to be "The type specification statement must not specify "
          "a type with a SEQUENCE attribute or a BIND attribute"


================
Comment at: flang/lib/Semantics/check-select-type.cpp:183
+  // checks for kinds as well.
+  static bool Comparator(const TypeCase &x, const TypeCase &y) {
+    if (x.IsDefault()) { // C1164
----------------
I'd prefer a more evocative name, such as "TypesAreDifferent()"


================
Comment at: flang/lib/Semantics/check-select-type.cpp:194
+
+  static bool CheckTypeKindCompatibility(const TypeCase &x, const TypeCase &y) {
+    return (*x.guardType()).IsTkCompatibleWith((*y.guardType()));
----------------
I'd prefer a more evocative name such as "AreTypeKindCompatible()"


================
Comment at: flang/lib/Semantics/check-select-type.cpp:198
+
+  //  // This has quadratic time, only runs in non-error cases
+  void ReportConflictingTypeCases() {
----------------
There's an extra "//" here


================
Comment at: flang/lib/Semantics/check-select-type.cpp:207
+            msg = &context_.Say(iter->stmt.source,
+                "Type specification %s conflicts with previous type specification"_err_en_US,
+                iter->AsFortran());
----------------
There should be single quotes around the name of the conflicting type specification -- as in "Type specification '%s' conflicts with previous type specification"_err_en_US,


================
Comment at: flang/lib/Semantics/check-select-type.cpp:210
+          }
+          msg->Attach(p->stmt.source, "Conflicting type specification %s"_en_US,
+              p->AsFortran());
----------------
There should be single quotes around the name of the conflicting type specification -- as in "Conflicting type specification '%s'"_en_US.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5101-5102
+
+    } else if (evaluate::HasVectorSubscript(
+                   *association.selector.expr)) { // C1158
+      Say(association.selector.source,
----------------
C1158 also applies to selectors that are not variables.  Can you please add a check and test for that situation?


================
Comment at: flang/test/Semantics/selecttype01.f90:1
+! RUN: %B/test/Semantics/test_errors.sh %s %flang %t
+! Test for checking select type constraints,
----------------
Can you please add tests for C1165?


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

https://reviews.llvm.org/D79851