[PATCH] D102210: [flang] Allow large and erroneous ac-implied-do's

[Jean Perier via Phabricator via llvm-commits]

jeanPerier accepted this revision.
jeanPerier added inline comments.
This revision is now accepted and ready to land.


================
Comment at: flang/include/flang/Evaluate/shape.h:187
+          // large we can't evalute it without overflowing the stack.
+          result = Fold(*context_, std::move(result));
+        }
----------------
Are they not cases where the additions cannot be folded and the array constructor could still grow big ?
If so, then we will still eventually hit the overflow, and every Fold might actually get expensive.

Maybe `GetArrayConstructorExtent` should give up if the non constant part grows too much (this could be done by keeping constants `n` and non constants `n` in a different accumulators for which the number of operands would be tracked, and  `GetArrayConstructorExtent` would return nullopt after a certain threshold is reached).

I am not sure if there are actually cases where non constant `n` could still lead to an overflow (I suppose ac-implied-do containing such dynamic extents expression would not be unrolled). So what you have may be enough until we hit a scenario where more is needed here.


================
Comment at: flang/lib/Parser/message.cpp:330
   for (const Message *msg : sorted) {
+    if (lastMsg && *msg == *lastMsg) {
+      // Don't emit two identical messages for the same location
----------------
I am not opposed to having this here. I am wondering if the places that are raising the same messages over and over should not also be  be prevented from doing so. Maybe some checkers in ArrayConstructors should stop after x errors to avoid wasting time and filling `messages_`.
Again, if you considered it and it turned out unpractical, what you have here already looks like an improvement to me, and it had the benefit to clean some other small error duplication cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102210