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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski 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
 };
----------------
klausler wrote:
> 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.
`prescanAndReformat`?


================
Comment at: flang/lib/Parser/parsing.cpp:104
+  int column{1};
+  static int constexpr columnLimit{72};
+  bool inDirective{false};
----------------
klausler wrote:
> 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.
Makes sense. Could you add a comment then? Otherwise it's just a magic number. A comment will prevent folks (including my future self) from changing this to `Fortran::parser::Options::fixedFormColumns`.


================
Comment at: flang/lib/Parser/parsing.cpp:138
+          if (sourceLine < position->line &&
+              sourceLine + 10 >= position->line) {
+            while (sourceLine++ < position->line) {
----------------
klausler wrote:
> 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).
[nit] A non-magic number would be great (e.g. `unsigned newLineCharLitmit = 10`). Or a comment.


================
Comment at: flang/lib/Parser/parsing.cpp:155
+        inContinuation = true;
+      } else if (fixedForm && !inDirective && column < 7 && ch != ' ' &&
+          (ch < '0' || ch > '9')) {
----------------
Non-digit in the first 6 columns (fixed form) is an error, right? Why not assert or issue a diagnostic here? This could hide a potential bug.


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

https://reviews.llvm.org/D106727