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

[Peter Klausler via Phabricator via flang-commits]

klausler requested changes to this revision.
klausler added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:41
+    } else if (std::optional<evaluate::DynamicType> type{GetGuardType(guard)}) {
+      auto success{PerformChecksOnGuard(guard, *type)};
+      if (success) {
----------------
`success` is not necessary.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:63
+              if (const DeclTypeSpec * declTypeSpec{typeSpec.declTypeSpec}) {
+                if (std::optional<evaluate::DynamicType> guardTypeDynamic{
+                        evaluate::DynamicType::From(declTypeSpec)}) {
----------------
Just `return evaluate::DynamicType::From(typeSpec.declTypeSpec);` for the whole function body.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:73-83
+                const semantics::Symbol &typeSymbol{
+                    derivedTypeSpec->typeSymbol()};
+                if (const semantics::Scope * scope{typeSymbol.scope()}) {
+                  auto &mutablederivedTypeSpec{
+                      *const_cast<semantics::DerivedTypeSpec *>(
+                          derivedTypeSpec)};
+                  auto &mutableConstScope{
----------------
Your code code has no comments anywhere to explain what you're doing, and they would be helpful here.  It looks like you're using move semantics to suck the contents out of the derived type spec and transfer them to a new derived type spec in the scope of the original derived type spec.  This seems likely to wreck the representation of the original type, and I can't tell why you want to do it.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:84
+                      std::move(mutablederivedTypeSpec))};
+                  if (auto guardTypeDynamic{
+                          evaluate::DynamicType::From(declTypeSpec)}) {
----------------
Just `return`; the `if` is not necessary.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:128
+  }
+  bool ChecksOnDerivedTypeSpec(const semantics::DerivedTypeSpec *derived,
+      parser::CharBlock sourceLoc) const {
----------------
If `derived` can't be null, use a reference.  If it can be null, check it.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:139
+  }
+  struct TypeCase {
+    explicit TypeCase(const parser::Statement<parser::TypeGuardStmt> &s,
----------------
Is `struct TypeCase` nested in `class TypeCaseValues`?


================
Comment at: flang/lib/Semantics/check-select-type.cpp:178
+      std::string result;
+      llvm::raw_string_ostream bs{result};
+      if (this->guardType()) {
----------------
You're just coping a single string to a stream to get the same string back.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:194
+  // checks for kinds as well.
+  struct Comparator {
+    bool operator()(const TypeCase &x, const TypeCase &y) const {
----------------
Should be a function, not a function object; it has no state, and is only called directly.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:195
+  struct Comparator {
+    bool operator()(const TypeCase &x, const TypeCase &y) const {
+      if (x.IsDefault()) { // C1164
----------------
Can be `static` rather than `const`, I think.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:208
+        const TypeCase &x, const TypeCase &y) const {
+      if (x.guardType() && y.guardType()) {
+        auto typex{*x.guardType()};
----------------
These six lines could be a single `return` statement.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:217
+
+  //  // This has quadratic time, but only runs in non-error cases
+  void ReportConflictingTypeCases() {
----------------
The "but" here isn't reassuring, since the cases where compilation time matters are non-error cases.


================
Comment at: flang/lib/Semantics/check-select-type.cpp:240
+  bool hasErrors_{false};
+}; // namespace Fortran::semantics
+
----------------
I don't think that this closing brace is ending the namespace, but rather a class.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5096
+          Say(association.selector.source,
+              "Selector '%s' in SELECT TYPE statements must be "
+              "polymorphic value"_err_en_US);
----------------
s/statements/statement/

s/ value//


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5103
+                   *association.selector.expr)) { // C1158
+      Say(association.selector.source,
+          "Neither 'associate-name =>' "
----------------
This error message is unclear.  Explain why the expression cannot be used with an associate-name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79851