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

[Pete Steinfeld via Phabricator via llvm-commits]

PeteSteinfeld 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 *>;
+
----------------
tskeith wrote:
> `symbol.h` defines `SymbolSet` which you could use here.
Thanks, I'll do that.


================
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;
----------------
tskeith wrote:
> 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.
Good suggestion.  And even though the Characterize() methods of DummyArgument and DummyProcedure aren't overloads, It seems to me that they should also be made static in characteristics.cpp.


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


================
Comment at: flang/lib/Evaluate/characteristics.cpp:659
+  }
+  return result;
+}
----------------
tskeith wrote:
> FYI, llvm has utility functions in `llvm/ADT/STLExtras.h` that simplify this kind of thing. If you take my suggestion of enclosing each procedure name in quotes you could implement this function as:
> ```
>   llvm::interleave(
>       seenProcs,
>       [&](const Symbol *p) { result += '\'' + p->name().ToString() + '\''; },
>       [&]() { result += ", "; });
> ```
How do you find this stuff?  I'll do it.


================
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)
----------------
tskeith wrote:
> I think the procedure names should be quoted individually, i.e. `'sub', 'p2'`.
Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96631