[flang-commits] [PATCH] D155499: [flang] Stricter "implicit continuation" in preprocessing

[Roger Ferrer Ibanez via Phabricator via flang-commits]

rogfer01 accepted this revision.
rogfer01 added a comment.

LGTM. I suggest adding an XFAIL'd test but I'll leave this at your choice.



================
Comment at: flang/include/flang/Parser/char-block.h:67
 
+  char OnlyNonBlank() const {
+    char result{' '};
----------------
klausler wrote:
> rogfer01 wrote:
> > klausler wrote:
> > > rogfer01 wrote:
> > > > I'm confused by this function. I've tested it with several inputs and it always seems to return `' '`?
> > > > 
> > > > My tests here: https://www.godbolt.org/z/af9oc74aE
> > > > 
> > > > Maybe I'm missing something. Can you add a comment about its purpose?
> > > It returns the only non-blank character, if it is the only non-blank character.
> > Ah, right a case I forgot to test 😅  .  Thanks.
> > 
> > Would it make sense not to use a loop? Something like this:
> > 
> > ```lang=cpp
> > char OnlyNonBlank() const {
> >     if (size() == 1) {
> >         char ch{*begin()};
> >         if (ch != ' ' && ch != '\t') {
> >             return ch;
> >         }
> >     }
> >     return ' ';
> > }
> > ```
> The entire point of this function is to ignore leading and trailing blanks to expose a single-character token.  Every character needs to be examined.  This requires a loop.
Right. Thanks for the clarification!


================
Comment at: flang/lib/Parser/preprocessor.cpp:1198
+    }
+    anyMacroWithUnbalancedParentheses_ = nesting != 0;
+  }
----------------
klausler wrote:
> rogfer01 wrote:
> > `anyMacroWithUnbalancedParantheses_` is only getting updated when we find a directive and we do not seem to clean it up, so we end carrying wrong state up to the point where we decide we allow an implicit continuation.
> > 
> > Consider the following testcase:
> > 
> > ```lang=fortran
> > program main
> >   implicit none
> > #define FOO(x) ((x) + 1)
> > #define BAR (
> >   integer :: k
> > 
> >   k = FOO(
> >     4)
> > end program main
> > ```
> > 
> > This fails with the patch
> > 
> > ```
> > $ flang-new -E -o- t.F90 
> > error: Could not scan t.F90
> > ./t.F90:8:10: error: Unmatched '('
> >     k = FOO(
> >            ^
> > ./t.F90:9:6: error: Unmatched ')'
> >       4)
> >        ^
> > ```
> > 
> > but if you remove (or comment) the definition of `BAR` the file is scanned correctly.
> > 
> > ```
> > flang-new -E -o- t.F90 
> > #line "./t.F90" 2
> >       program main
> >       implicit none
> > 
> > 
> >       integer :: k
> > 
> >       k = (( 4) + 1)
> > 
> >       end program main
> > ```
> The whole point of this flag is to disable all implicit continuations if any macro definition has unbalanced parentheses.  I can't figure out how to do it safely and I've spent too much time on this topic already.
Right. My concern is that the definition of a macro with unbalanced parentheses that it is not even used in the code causes a later implicit continuation to be disabled.

This is a bit surprising and I think it may be confusing. But it seems an unusual situation so I understand its impact is low and can be fixed later. I suggest to add a `XFAIL` testcase for it at least.

(gfortran and pgf90 don't have a problem with this. ifort/ifx rejects that but that seems because it does not support this kind of implicit continuations.)



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

https://reviews.llvm.org/D155499