[flang-commits] [PATCH] D106727: [flang] Produce proper "preprocessor output" for -E option

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

In D106727#2904725 <https://reviews.llvm.org/D106727#2904725>, @klausler wrote:

> I was hoping to get a code review.  Tests will be added before submission.

I read it and it made a lot of sense to me. It's fairly straightforward, like you suggested it would.  The commit message makes it clear _what_ and _why_ is happening and that's consistent with what the code does. Glad to see `-fdebug-dump-cooked-chars` and `-P` being introduced. The latter is consistent with `gfortran`, which is important for us. This change should be sufficient for us to make progress with CMake.

I downloaded and tested your patch - hence my remark about the tests (they failed for me). Also, extra tests would really help to identify potential corner cases.  Anyway, I scanned the implementation again and left a few inline comments.

> The link you gave to bugzilla does not work.

Sorry, my bad: https://bugs.llvm.org/show_bug.cgi?id=51219.



================
Comment at: flang/include/flang/Parser/parsing.h:40
   Fortran::parser::Encoding encoding{Fortran::parser::Encoding::UTF_8};
+  bool prescanOnly{false}; // -E
 };
----------------
[nit] Why not preprocess only? This is triggered by `-E` and does a bit more than just prescanning. But I don't mind.


================
Comment at: flang/lib/Parser/parsing.cpp:104
+  int column{1};
+  static int constexpr columnLimit{72};
+  bool inDirective{false};
----------------
This should be `Fortran::parser::Options::fixedFormColumns` rather than hard-coded `72`.


================
Comment at: flang/lib/Parser/parsing.cpp:136
+              << '\n';
+        } else if (position->line != sourceLine) {
+          if (sourceLine < position->line &&
----------------
Could you add an assert here `assert(position->line > sourceLine && "sourceLine exceeded the actual source line")`? There are not too many comments here and asserts can be very informative. 


================
Comment at: flang/lib/Parser/parsing.cpp:138
+          if (sourceLine < position->line &&
+              sourceLine + 10 >= position->line) {
+            while (sourceLine++ < position->line) {
----------------
Why `10` and why make this case special?


================
Comment at: flang/lib/Parser/parsing.cpp:149
+      }
+      if (column > columnLimit) {
+        // Wrap long lines in a portable fashion
----------------
This will only make sense for fixed-form, right?


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

https://reviews.llvm.org/D106727