[PATCH] D96631: [flang] Detect circularly defined procedures

[Tim Keith via Phabricator via llvm-commits]

tskeith added inline comments.


================
Comment at: flang/include/flang/Evaluate/characteristics.h:57
+// 15.4.3.6 paragraph 2
+using SeenProcs = std::set<const semantics::Symbol *>;
+
----------------
`symbol.h` defines `SymbolSet` which you could use here.


================
Comment at: flang/include/flang/Evaluate/characteristics.h:211
   static std::optional<DummyProcedure> Characterize(
-      const semantics::Symbol &, FoldingContext &context);
+      const semantics::Symbol &, FoldingContext &context, SeenProcs &);
   llvm::raw_ostream &Dump(llvm::raw_ostream &) const;
----------------
PeteSteinfeld wrote:
> klausler wrote:
> > As a convenience to external clients, this new argument could be either a pointer that default to nullptr, or there could be an overload without the set argument that declares one and passes it onward (my preference).
> I'm not sure if I understand your comment.  There are no external clients for the Characterize() functions for a DummyProcedure or a DummyArgument.  Rather, all of the calls to them happen in .../Evaluate/characteristics.cpp.  There are external clients for Procedure::Characterize().  For this function, I did as you recommend -- the version without the set argument creates one and passes it on to the overload that contains the set argument.
The new overloads that include `SeenProcs` aren't used by external clients so perhaps they can be static non-member functions in `characteristics.cpp`. Then the external API won't change at all.


================
Comment at: flang/lib/Evaluate/characteristics.cpp:648
 
+static std::string GetSeenProcs(SeenProcs &seenProcs) {
+  std::string result;
----------------
This could be `const SeenProcs &`.


================
Comment at: flang/lib/Evaluate/characteristics.cpp:651
+  bool firstTime{true};
+  for (auto &procPtr : seenProcs) {
+    if (!firstTime) {
----------------
clang-tidy says this should be `const auto &procPtr`. But `const auto *procPtr` would be better.


================
Comment at: flang/test/Semantics/resolve100.f90:4
+! Tests for circularly defined procedures
+!ERROR: Procedure 'sub' is recursively defined.  Procedures in the cycle: 'sub, p2'
+subroutine sub(p2)
----------------
I think the procedure names should be quoted individually, i.e. `'sub', 'p2'`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96631