Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support selected Intel-specific preprocessor macro expansions #341

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

emanspeaks
Copy link
Contributor

@emanspeaks emanspeaks commented Dec 11, 2023

Closes issue 297
Addresses spaces on either side of the hash in preprocessor lines
(Edit: this PR has been repurposed after refactoring. The previously-cited description now applies to #350.)

Addresses Intel compiler extensions present due to legacy support for older compiler versions. These are particularly prolific in their implementation of Fortran standard library modules and system/platform-specific modules, but for which it would be desired to have hover capabilities for those users working in the Intel ecosystem.

fortls/regex_patterns.py Fixed Show fixed Hide fixed
fortls/regex_patterns.py Fixed Show fixed Hide fixed
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (71d5f40) 87.51% compared to head (c975006) 87.79%.

Files Patch % Lines
fortls/langserver.py 88.88% 1 Missing ⚠️
fortls/parsers/internal/parser.py 98.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   87.51%   87.79%   +0.27%     
==========================================
  Files          35       35              
  Lines        4756     4824      +68     
==========================================
+ Hits         4162     4235      +73     
+ Misses        594      589       -5     

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

fortls/regex_patterns.py Fixed Show fixed Hide fixed
fortls/regex_patterns.py Fixed Show fixed Hide fixed
@emanspeaks emanspeaks changed the title Allow for whitespace in PP directives Allow for whitespace in PP directives and full macro expansions Jan 3, 2024
@emanspeaks emanspeaks marked this pull request as ready for review January 3, 2024 07:57
@emanspeaks
Copy link
Contributor Author

I'm not sure what to do about the warning on the regex above, but otherwise this seems to be working like a champ. I look forward to your comments and/or your merging hopefully soon :-)

@gnikit
Copy link
Member

gnikit commented Jan 3, 2024

I see a lot of changes that are seemingly unrelated in this PR. My expectation would be that you would only need to add [ ]* to some regex expressions? Can you elaborate as to the other modifications?

@emanspeaks
Copy link
Contributor Author

I buried the lede perhaps, but did rename the PR to say it is performing full macro expansions for function-like macros. That was actually the intent of this PR and the whitespace changes were just a bonus. I will change the name again to be even more explicit about what is going on here. This resulted in a need for updated/new unit tests, which are the remaining updates.

In order to also support lines containing tabs in addition to spaces, I chose to use \s* instead of [ ]*. Those changes should be obvious in regex_patterns.py.

@emanspeaks emanspeaks changed the title Allow for whitespace in PP directives and full macro expansions Support full function-like macro expansions and allow for whitespace in PP directives Jan 3, 2024
@emanspeaks
Copy link
Contributor Author

Sorry, should have included this in my last message, but let me elaborate on specifically how I do this. The updated PP_DEF regex now captures parenthesis if they exist next to a macro name. These are captured in regex group 3 (inclusive of parentheses). Group 4 is the contents of the parens (i.e. the arguments of the macro function).

If regex group 3 contains a match, parens are present in the macro name, and rather than just adding the string of the #define's "value" to the pp_defs dict, it adds a tuple of the arguments (group 4) and THEN the rest of the line.

In later code that parses out values from pp_defs dicts, I add code to detect if the contents are a tuple rather than simple string. If so, then the expansion is performed as necessary. Otherwise, it behaves just as before.

I searched the code for everywhere it seemed like pp_defs or something similar were being referenced, so all the bases should be covered on handling the tuple where previously it was only a string.

I added unit tests for these expansions as well as testing for additional whitespace in PP directives, which themselves broke other unit tests, and I made a number of changes as a result of expected changes due to the new unit test and code changes. The only test that continued to fail was one regarding server update, which was the result of my running the tests in a conda environment. It is expected that the update will not be performed when tests are run in a conda environment, so that test checks if the test environment is a conda environment and changes the expected test value if so. Another Easter egg update I forgot about until reviewing the changes again just now, ha ha.

@gnikit
Copy link
Member

gnikit commented Jan 3, 2024

In order to also support lines containing tabs in addition to spaces, I chose to use \s* instead of [ ]*. Those changes should be obvious in regex_patterns.py.

\s contains a lot more that just and \t characters, see and that is a problem. The rest of the parser does not support tabs, so allowing for tabs in a small section of the parser is a separate issue and a separate PR. For now whitespaces should be used.

I will have a detailed look at the PR but I am not sure the proposed solution works for all cases

@emanspeaks
Copy link
Contributor Author

I can replace the \s with spaces, happy to do that.

At a minimum, this implementation should be consistent with Intel and gfortran conventions. I can't speak to any other preprocessors. I am happy to continue improvement to support others if there are cases that this doesn't work, I just need test cases for those and I can add to the unit tests. Now that this infrastructure is in place for doing the expansions, it shouldn't difficult to tweak as needed for other edge cases.

fortls/regex_patterns.py Fixed Show fixed Hide fixed
fortls/regex_patterns.py Fixed Show fixed Hide fixed
fortls/regex_patterns.py Fixed Show fixed Hide fixed
@emanspeaks
Copy link
Contributor Author

I made the requested update above to only check for spaces. I have subsequently come across some code that depended on Intel's compiler directives, so I added an option here to support that (defaults to disabled, Intel users would need to opt in).

@gnikit
Copy link
Member

gnikit commented Jan 14, 2024

I haven't forgotten about this @emanspeaks, I have a mostly-finished review (that is now outdated since the last push). It is in my TODO list and I will try and finish today, but I am quite busy (so no promises) I will make it 😟

@gnikit
Copy link
Member

gnikit commented Jan 16, 2024

I've noticed the PR now includes Intel-specific options. Let's break down these features into separate PRs to keep things manageable and focused.

Intel Options

Remember, our language server is vendor-neutral. We can't start adding compiler-specific features; it's not sustainable. If Intel directives fit with our current preprocessor, great, let's include them. If not, we can't realistically rewrite the preprocessor for both cpp and fpp. So, these changes might not be feasible for fortls.

We should discuss this further, but as a separate issue and PR.

For this PR, what are the features that are vendor agnostic and are responsible for the macro expansion? These should be the focus. Let's cherry-pick and either update this PR or open a new one with only these features.

@emanspeaks
Copy link
Contributor Author

Yeah, this morphed rather organically from where this branch started. I agree it makes sense to split this up. It shouldn't be terribly difficult to take care of, and will try to do that this weekend.

@emanspeaks emanspeaks changed the title Support full function-like macro expansions and allow for whitespace in PP directives Support selected Intel-specific preprocessor macro expansions Jan 21, 2024
@emanspeaks
Copy link
Contributor Author

emanspeaks commented Jan 21, 2024

Given that the name of this branch I'm working from here is named ifort-fpp-fixes, it made more sense to me to use that branch for the vendor-specific changes for Intel. GitHub won't let you change the branch associated with a PR, but I didn't want to close this one either. If you'd rather I do that and we get a clean start with this branch in a new PR, I can do that instead.

Assuming, though, it's ok to proceed here, let me reconcile this branch now with the forked changes in #350 and will let you know when this is ready for review again.

@emanspeaks
Copy link
Contributor Author

Ok, I think the changes are complete. Since this branch contains both its own contents and #350 's changes, looking at the changed files here isn't the most helpful until/unless you merge that one first. If you want to see the Intel-specific changes relative to #350, you can look here

gnikit pushed a commit that referenced this pull request Mar 30, 2024
gnikit pushed a commit that referenced this pull request Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants