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

[Peter Klausler via Phabricator via flang-commits]

klausler marked 5 inline comments as done.
klausler added inline comments.


================
Comment at: flang/include/flang/Parser/parsing.h:40
   Fortran::parser::Encoding encoding{Fortran::parser::Encoding::UTF_8};
+  bool prescanOnly{false}; // -E
 };
----------------
awarzynski wrote:
> [nit] Why not preprocess only? This is triggered by `-E` and does a bit more than just prescanning. But I don't mind.
>From the perspective of the Parsing module, we're just going to run the prescanner.  F18 has no preprocessing phase; there is no way to just run its preprocessor.  If there's a better accurate name that you would prefer, perhaps "preprocessedOutputOnly", let me know.


================
Comment at: flang/lib/Parser/parsing.cpp:104
+  int column{1};
+  static int constexpr columnLimit{72};
+  bool inDirective{false};
----------------
awarzynski wrote:
> This should be `Fortran::parser::Options::fixedFormColumns` rather than hard-coded `72`.
There's no guarantee that a later Fortran compilation of the -E output will also be using a nonstandard fixed form statement length.  72 is safer.


================
Comment at: flang/lib/Parser/parsing.cpp:136
+              << '\n';
+        } else if (position->line != sourceLine) {
+          if (sourceLine < position->line &&
----------------
awarzynski wrote:
> 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. 
That case could arise in some future where f18 honors `#line` directives in its input.


================
Comment at: flang/lib/Parser/parsing.cpp:138
+          if (sourceLine < position->line &&
+              sourceLine + 10 >= position->line) {
+            while (sourceLine++ < position->line) {
----------------
awarzynski wrote:
> Why `10` and why make this case special?
Emitting a few newlines can be more readable and occupy fewer bytes than the equivalent `#line` directive would have required.  The 10 is the number of bytes in a `#line 123` directive (with newline).


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


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

https://reviews.llvm.org/D106727