[flang-commits] [PATCH] D78623: [flang] Semantic checks for SELECT RANK

[Tim Keith via Phabricator via flang-commits]

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


================
Comment at: flang/include/flang/Semantics/symbol.h:149
+  void set_rank(int rank);
+  std::optional<int> associationRank() const { return associationRank_; }
 
----------------
I think `rank` and `rank_` are better names than `associationRank` and `associationRank_`.
You know it's the rank of an association because it is in an `AssocEntityDetails`, and that would be consistent with `set_rank`.


================
Comment at: flang/lib/Evaluate/shape.cpp:473
+    }
+    return Result{shape};
   } else {
----------------
Can you extract all of the copies of this code into a single function?


================
Comment at: flang/lib/Semantics/check-select-rank.cpp:69
+                    "Not more than one of the selectors of SELECT RANK "
+                    "statement may be default"_err_en_US);
+              }
----------------
You could show where the previous DEFAULT is.


================
Comment at: flang/lib/Semantics/check-select-rank.cpp:78
+                    "Not more than one of the selectors of SELECT RANK "
+                    "statement may be '*'"_err_en_US);
+              }
----------------
Same here.


================
Comment at: flang/lib/Semantics/check-select-rank.cpp:115
+
+const SomeExpr *SelectRankConstructChecker::ResolveSelector(
+    const parser::Selector &selector) {
----------------
I think `GetExpr` is a better name for this function. Nothing is being resolved.


================
Comment at: flang/lib/Semantics/check-select-rank.cpp:123
+      selector.u);
+}
+
----------------
You should be able to write this as:
`std::visit([](const auto &x) { return GetExpr(x); }, selector.u);`

Or maybe change `GetExpr` to operate on any union class as it does on wrapper classes.


================
Comment at: flang/lib/Semantics/resolve-names.cpp:5147
+    if (const auto *init{std::get_if<parser::ScalarIntConstantExpr>(&x.u)}) {
+      Walk(*init);
+      MaybeIntExpr expr{EvaluateIntExpr(*init)};
----------------
Why do you need this `Walk`? Didn't it just get visited?


================
Comment at: flang/lib/Semantics/symbol.cpp:290
 
+void Symbol::SetRank(int rank) {
+  if (auto *Details{std::get_if<AssocEntityDetails>(&details_)}) {
----------------
I don't think this belongs in `Symbol`. It is only called in one place and it's not consistent with `GetRank`. You can get the rank of all kinds of symbols but you can set it on only one particular kind.


================
Comment at: flang/lib/Semantics/symbol.cpp:292
+  if (auto *Details{std::get_if<AssocEntityDetails>(&details_)}) {
+    Details->set_rank(rank);
+  } else {
----------------
`Details` should be `details`.
Even better, use `get<AssocEntityDetails>()` and the check will be done automatically.


================
Comment at: flang/test/Semantics/select-rank.f90:75
+      print *, "ok "
+    !ERROR: Not more than one of the selectors of SELECT RANK statement may be default
+    RANK DEFAULT
----------------
"default" -> "DEFAULT"


================
Comment at: flang/test/Semantics/select-rank.f90:92
+      print *, "PRINT RANK 1"
+    !ERROR: Same rank values not allowed more than once
+    RANK(0)
----------------
This message could say what the bad value is. If it's an expression it may not be obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78623