Fix preprocessor else and elif

[AljenU]

The handling of preprocessor else and elif statements has been updated, so their contents are now correctly included (or excluded) depending on whether any previous (el)if in the if-(elif-)else branching has been true.

[codecov]

Codecov Report

Merging #331 (dc3449e) into master (27b9d49) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   87.10%   87.54%   +0.43%     
==========================================
  Files          12       12              
  Lines        4551     4567      +16     
==========================================
+ Hits         3964     3998      +34     
+ Misses        587      569      -18     

Files	Coverage Δ
fortls/parse_fortran.py	90.70% <100.00%> (+1.58%)	⬆️

[gnikit]

What bug/feature is this addressing, can you describe it please

[AljenU]

What bug/feature is this addressing, can you describe it please

Multiple problems, running the first commit without the second will show what goes wrong. In short:

When there are preprocessor directives with logical branching, the preprocessor parsing would skip any #define and #include statements in the #else part of the logical branch, regardless of whether the #if part was TRUE or FALSE.

When there was an #elif that evaluated to TRUE, and all previous #if or #elif were FALSE, that previous part of the file was NOT added to the list of lines that needed to be skipped (and if it had been, the ending line number was 2 too small).

When evaluating whether the code inside an #elif branch, or the final #else branch, should be included, the parsing did not take into account whether an earlier #if or #elif had already been true, except for the directly-preceding branch.

When at any point an #elif evaluated to TRUE, the whole file after the end of that branch would not be parsed for #define or #include statements (because the stack variable in parse_fortran would never be empty again).

[gnikit]

In that case could we open Issues to accompany the PR. I am in favour of merging this, but I prefer making one change/fix at a time.
Could I also ask to update the base branch?
Also, is one of your changes/fixes here related to 1. in this PR #277

[AljenU]

Yes, this does address also item 1 from #277.

I can try to split up into multiple commits, the #else change can be separate at least. Amd maybe the others also, although they are quite connected in that #elif parsing was just not ok.

Does it need to be separate PRs even, or can it be one PR with multiple commits, each commit fixing as little as possible?

And, I have now separated the test case addition from the fix commit, so you can checkout the test case commit to see what goes wrong, and then add the fix commit to see that it then goes ok. Is that an ok workflow, or do you prefer the test case and the fix to be in one commit?

[gnikit]

Yes, this does address also item 1 from #277.

Ok, awesome thanks.

Does it need to be separate PRs even, or can it be one PR with multiple commits, each commit fixing as little as possible?

Multiple commits are equally fine in this case, since the git diffs are quite small. We just need to have a way to revert a "single" change/fix.

And, I have now separated the test case addition from the fix commit, so you can checkout the test case commit to see what goes wrong, and then add the fix commit to see that it then goes ok. Is that an ok workflow, or do you prefer the test case and the fix to be in one commit?

Yes, that is fine. I will have a look at it and let you know.

[AljenU]

The changes are split up into multiple commits now, with a test and a fix for each problem.

[gnikit]

Fixes #332 via 294e6b8 and cc759bc

[gnikit]

I have had a look at the first 6 commits and they look good. Commits d18b2ea and 0e3baec I am still trying to process what is going on. That test is too big ~140 lines and rather cryptic, which makes it hard to maintain in the future. I will leave comments with recommendations to make it more readable.

[AljenU]

I have had a look at the first 6 commits and they look good. Commits d18b2ea and 0e3baec I am still trying to process what is going on. That test is too big ~140 lines and rather cryptic, which makes it hard to maintain in the future. I will leave comments with recommendations to make it more readable.

I agree it is a big test file, I just could not think on how to test the problem+fix another way with valid fortran code. If you can come up with something that needs less define, if-not-defined and undef statements that would be great.

The first part, with var1 and var2, is a regression test for the whole-file-ignored and the wrong-indexing problems.

The main point of everything after that, with var3 - var6 is that when there is an if-elif-elif-else, and multiple conditions evaluate to true, only the first branch that evaluates to true should be used, and the others ignored.

Additionally, to have code coverage on all paths in the parsing code, there needed to be testcases with different orders of truthiness of the branches in the if-elif-elif-else. Thus, each of those 4 parts are basically the same code, with just a difference in which of the conditions is true. Unfortunately, each part is a lot of lines, to have valid-fortran-but-wrong-content code when there is a bug in which branches are used, that can then be easily compared with the valid-fortran-and-correct-content when the parsing is done correctly.

EDIT: While typing this: it might be possible to put the repeated content w.r.t. the defines and if-not-defineds in an include file, and set the thruthiness of the branches through other defines in the main file. Then include the file multiple times, with different truthiness values before each include. That would reduce the amount of code in the test file.

[gnikit]

Don't worry I have a half-finished review from the weekend with some recommendations on how to improve. I will see if I can finalise tonight.

[gnikit]

Thanks for the fix, I have left some comments/ideas on how to split that single source file into 3 separate files. The coverage tests between the two versions appeared to be the same for me.

For convinience feel free to git cherry-pick 3f87db0cadefd9ffea76916c5de6a62f251733ff. Commit 3f87db0 contains the changes I proposed in the unit test.

[gnikit]

This looks good

FYI the #undef where there at the end because I stumbled onto some unexpected behaviour where edits to the file would not propagate to the preproc definitions but I didn't have time to investigate so I just patched it. If you save the file, make some modifications to the values of the preprocessor variables and then save again, you should be getting the wrong values during hover.