390 ignore linemarkers by hiker · Pull Request #498 · stfc/fparser

hiker · 2026-04-13T03:07:17Z

Fixes #390 by supporting linemarkers (# 123 "test.f90" - indicating line number and file).

I have used the existing preprocessor directive support, since linemarkers looks pretty much like a preprocessor directive (starting with #). But I was unable to get it to verify properly out of the box - I feel like the existing implementation assumes that the value is constant (e.g. #ifdef), while in the case of a linemarker we don't have this (since it would need the line number and filename to verify correctness, and I couldn't work out how to do this). While it worked this way, it was unable to detect invalid linemarkers (# abc "xx.f90" or # 123 'no_single_quote_allowed.f90'). So, instead I added a simple regex check at the beginning, and only if that passes, it will use the established path.

I am happy to reimplement (I know there is pattern_tools.digit_string and file_name ... I just couldn't figure out how to plug this all together.

…kers.

hiker · 2026-04-13T03:08:29Z

I could also not find an official syntax for a linemaker, but looked at https://git.cels.anl.gov/ksalapenades/llvm-project/-/blob/develop/clang/test/Preprocessor/line-directive.c (and did some compiler tests to verify what gets rejected)

codecov · 2026-04-13T03:10:34Z

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.21%. Comparing base (7d396b6) to head (773ff8a).

Files with missing lines	Patch %	Lines
src/fparser/two/C99Preprocessor.py	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #498   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files          87       87           
  Lines       13831    13845   +14     
=======================================
+ Hits        12754    12767   +13     
- Misses       1077     1078    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hiker · 2026-04-13T14:12:01Z

Checking the git log, it appears this code was contributed by @reuterbal - I wonder if you would have time to provide some feedback? Thanks!

reuterbal

Happy to provide my 2ct here. I agree that linemarkers should be handled like preprocessor directives and the implementation itself looks good to me.

As for the need for the additional regex validation: I believe you only need to encode the full linemarker specification as a regex pattern to use in WORDClsBase.match - either manually or using the building blocks in pattern_tools - e.g., like here:

fparser/src/fparser/two/pattern_tools.py

Line 416 in 5e124e5

part_ref = name + ~((r"[(]" + name + r"[)]"))

Without testing it, I would expect this would maybe need to look like something like this:

_pattern = r"^\s*#\s+" +  digit_string + "\s+" + file_name

reuterbal · 2026-04-14T12:56:25Z

+
+    # The match method will check that it is a valid linemarker, i.e.
+    # it has a line number, and file name in double quotes.
+    _pattern = pattern.Pattern("<linemarker>", r"^\s*#", value="#")


Note that most Fortran compilers don't accept white space in front of the # for preprocessor directives. Are they more forgiving for line markers?
But since fparser allows for white space in front of directives, it's probably wise to allow them for marker as well.

I actually remember that I was wondering about the \s at the beginning (since I did some testing originally, and knew it's not accepted). But since all regex here started with #\s* .. I just followed the trend :) And I'll agree to leave it in for consistency (and because I am afraid of breaking something).

…er syntax. Extend match function to be able to return the fully matched string.

hiker · 2026-04-16T14:03:17Z

Hi Balthasar,

thanks a lot for the quick review, really appreciated.

Happy to provide my 2ct here. I agree that linemarkers should be handled like preprocessor directives and the implementation itself looks good to me.

I am glad that I made the right decision here :)

As for the need for the additional regex validation: I believe you only need to encode the full linemarker specification as a regex pattern to use in WORDClsBase.match - either manually or using the building blocks in pattern_tools - e.g., like here:

fparser/src/fparser/two/pattern_tools.py

Line 416 in 5e124e5

part_ref = name + ~((r"[(]" + name + r"[)]"))

Without testing it, I would expect this would maybe need to look like something like this:
_pattern = r"^\s*#\s+" +  digit_string + "\s+" + file_name

I tried that, and thanks a lot for the example. Looking at the +digit_string..., made me realise that Pattern overwrites __add_-, but that doesn't work here since the first expression is a string. But I could use digit_string.pattern etc. But then file_name.pattern started with ^, so I couldn't use this. And since digit_string.pattern was pretty long with no apparent advantage over \d+, I actually just removed it and used a simple regex instead.

But then I am stuck with my original problem: I am using:

    _pattern = pattern.Pattern("<linemarker>", r"^\s*#\s+\d+\s+\".*\"")

But if I do this, the matching in WORDClsBase uses _pattern.value ... which is None. _pattern.value is a string, so I can set it to # (which I did in the state that you reviewed it), but then I don't get the rest of the match: The match function basically does:

           my_match = keyword.match(string)
            if my_match is None:
                return None
            line = string[len(my_match.group()) :]
            pattern_value = keyword.value
...

Basically, the rest of the match is discarded (line number and filename) :(

I tried to fix that using:

           my_match = keyword.match(string)
            if my_match is None:
                return None
            line = string[len(my_match.group()) :]
            if keyword.value:
                pattern_value = keyword.value
            else:
                pattern_value = my_match.group()

So, I provide value=None for the linemarker class, and that then triggers returning the fully matched string, including line numbers and filename.

This passed nearly all tests, except it would not remove spaces between "#" and line number and line number and file name. Therefore, I adjusted one linemarker test (but the test still checks that leading spaces before the # are removed, which is consistent with other code in C99_preprocessor). Small changes to __str__ were required.

I am not sure if this is the right solution tbh :(

Maybe it should not be based on WORDClsBase, but instead use the full grammar? But that also feels like an total overkill.

@reuterbal , @arporter , @sergisiso - feedback appreciated! The modification of the match function seems to have no other side effect, all tests were passing. But I am not keen on changing something that might be quite fundamental ;)

reuterbal

Nice work, the details of the keyword/value returns were lost on me and your proposal seems sensible to me.
Overall I think that's indeed much better now, no further comments!

arporter

Looks good to me Joerg, thanks. I could do with a bit of help understanding some of the changes - probably because I know pretty much nothing about the Pattern class.
I think it would also be good to test the new functionality in context - i.e. provide some Fortran containing some linemarkers and check that things work OK.

arporter · 2026-04-17T08:52:31Z


-"""C99 Preprocessor Syntax Rules."""
+"""C99 Preprocessor Syntax Rules. It also supports linemarker statements
+(which are technically not preprocessor directives, but are very close


Please also update the documentation (https://fparser.readthedocs.io/en/latest/fparser2.html#preprocessing-directives)

Oops, I missed that, thanks.

arporter · 2026-04-17T08:53:35Z


+class Cpp_Linemarker_Stmt(WORDClsBase):  # Linemarker
+    """
+    Linemarker


A bit more detail would be good here. i.e. explain what a Linemarker is.

arporter · 2026-04-17T08:55:55Z

+
+        :param str string: the string to match with as a line statement.
+
+        :return: a tuple of size 1 with the right hand side as a string, \


No line-continuation and please update to use type hints.

arporter · 2026-04-17T08:57:08Z

+    @staticmethod
+    def match(string):
+        """Implements the matching for a linemarker.
+        The right hand side of the directive is not matched any further


What does the "right hand side of the directive" mean?

I've reworded that.

arporter · 2026-04-17T08:58:12Z

+            require_cls=False,
+        )
+
+    def tostr(self):


typehints pls.

arporter · 2026-04-17T08:59:29Z

+        Returns the line marker as string. Note that fparser accepts
+        spaces before the `#`, but it should remove the spaces, hence
+        we lstrip the result
+        :return: this linemarker as a string.


Blank line before :return: please.

arporter · 2026-04-17T09:05:15Z

                return None
            line = string[len(my_match.group()) :]
-            pattern_value = keyword.value
+            # If no constant return value is defined,


I need a bit of help here: what does "no constant return value" mean?

I've added a longer comment.

…ed when they are in a Fortran program.

…parsed in utils as part of classes, not as part of add_comments_includes_directives anymore).

hiker · 2026-04-22T01:33:01Z

I've resolved Balthasar's comments, and addressed all new issues. Back for next review.

hiker · 2026-04-23T04:56:15Z

@arporter I just remembered that pylint flagged one issue, which looks wrong, but I just wanted to make sure it's not done on purpose before fixing. In test_c99preprocessor.py there is:

    [
...
        "#definex",
        "#define 2a" "#define fail(...,test) test",
        "#define",
        "#define fail(...,...)",
    ],
...
test_c99preprocessor.py:369:0: W1404: Implicit string concatenation found in list (implicit-str-concat)

Adding a , between the two strings works (as in: all tests are passing). It just looks wrong to me :)

The original (pre-black) line was:

@pytest.mark.parametrize('line', [
-    None, '', ' ', '#def', '#defnie', '#definex', '#define 2a'
-    '#define fail(...,test) test', '#define', '#define fail(...,...)'])

Which comes from d44ad584:

@@ -259,7 +259,7 @@ def test_macro_stmt_with_whitespace(line, ref):
 
 @pytest.mark.usefixtures("f2003_create")
 @pytest.mark.parametrize('line', [
-    None, '', ' ', '#def', '#defnie', '#definex',
+    None, '', ' ', '#def', '#defnie', '#definex', '#define 2a'
     '#define fail(...,test) test', '#define', '#define fail(...,...)'])
 def test_incorrect_macro_stmt(line):
     '''Test that incorrectly formed #define statements raise exception'''

Is it ok if I add the missing ,?

reuterbal · 2026-04-24T07:30:54Z

Nice catch, that was definitely a mistake when I expanded the test parameterisation. I'd say please add the missing ,.

…ngs.

hiker · 2026-04-28T02:14:44Z

Thanks for the confirmation. Updated to current master, ready for review.

hiker added 2 commits April 13, 2026 01:57

#390 Start to add support for linemarker.

ae41c50

#390 Improve linermarker handling to support detection of invalid mar…

5e124e5

…kers.

reuterbal reviewed Apr 14, 2026

View reviewed changes

hiker added 3 commits April 16, 2026 22:41

#390 Simplified parameterization.

7e482a2

#390 Fix incorrect linemarker specification in comment.

88a74ee

#390 Remove need for using an additional regex to verify the linemark…

8eb3954

…er syntax. Extend match function to be able to return the fully matched string.

reuterbal approved these changes Apr 16, 2026

View reviewed changes

arporter added enhancement under review labels Apr 17, 2026

arporter requested changes Apr 17, 2026

View reviewed changes

arporter added ready for review reviewed with actions PR has been reviewed and is back with developer and removed under review ready for review labels Apr 17, 2026

hiker added 6 commits April 22, 2026 10:38

#390 Updated manual.

fedc82c

#390 Updated comments and added proper typing.

8480ccf

#390 Added test that preprocessor directives are indeed properly pars…

b5e1f9f

…ed when they are in a Fortran program.

#390 Moved location of preprocessor directive (which means it is now …

76441b6

…parsed in utils as part of classes, not as part of add_comments_includes_directives anymore).

#390 Fixed black.

099a818

#390 Fix for github black, my black does not need that comma.

1634379

hiker added ready for review and removed reviewed with actions PR has been reviewed and is back with developer labels Apr 22, 2026

hiker requested a review from arporter April 22, 2026 01:33

hiker added 3 commits April 28, 2026 11:46

#390 Fix missing ',' which resulted in incorrectly concatenating stri…

27c7817

…ngs.

Merge remote-tracking branch 'origin/master' into 390_ignore_linemarkers

09099bf

#390 Fixed previous fix - oops.

773ff8a


		:param str string: the string to match with as a line statement.

		:return: a tuple of size 1 with the right hand side as a string, \

Conversation

hiker commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiker commented Apr 13, 2026

Uh oh!

codecov Bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hiker commented Apr 13, 2026

Uh oh!

reuterbal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hiker commented Apr 16, 2026

Uh oh!

reuterbal left a comment

Choose a reason for hiding this comment

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiker commented Apr 22, 2026

Uh oh!

hiker commented Apr 23, 2026

Uh oh!

reuterbal commented Apr 24, 2026

Uh oh!

hiker commented Apr 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hiker commented Apr 13, 2026 •

edited

Loading

codecov Bot commented Apr 13, 2026 •

edited

Loading