Refactor Platform class as Preprocessor class #199
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a much better name for the class, because it represents the state of a specific instance of a C preprocessor. The rename also makes it easier to justify why things like include paths and defines are an essential part of construction, allowing these to be moved into the __init__() function. Signed-off-by: John Pennycook <[email protected]>
define() previously took a name and a macro, despite a macro's name being encoded in the macro itself. undefine() and get_macro() expected a string, despite the rest of the preprocessor code using an Identifier token. Using the Identifier directly is less error-prone than having to remember to extract the string. Signed-off-by: John Pennycook <[email protected]>
is_defined() confusingly returned a string instead of a bool, and has_macro() is more aligned with the naming of get_macro(). This change also moves the handling of defined() into a single place, making it clearer that defined() will return a "1" or "0" wrapped in a NumericalConstant based on whether the macro exists. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
The naming of these functions was really unclear: - process_include didn't process includes, but rather returned a bool denoting whether an include file should be processed. - add_include_to_skip exposed an implementation detail (specifically, storing a "skip list") that wasn't obviously tied to usage (#pragma once). We can replace both of these functions with a new FileInfo concept that stores information about files, including whether a file has previously been marked as "include once". Signed-off-by: John Pennycook <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Platform class to Preprocessor, improving the naming consistency since "platform" is used differently elsewhere in the codebase. The refactoring also simplifies the API by using keyword-only arguments in the constructor and improving method signatures.
Key Changes:
- Renamed
Platformclass toPreprocessorwith updated imports across test files - Simplified constructor API with keyword-only parameters and type validation
- Updated method signatures to be more explicit (e.g.,
evaluate_for_platform→evaluate)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| codebasin/preprocessor.py | Main refactoring from Platform to Preprocessor class with improved API design |
| codebasin/finder.py | Updated imports and method calls to use new Preprocessor class |
| tests/operators/test_operators.py | Updated test to use new Preprocessor constructor with keyword arguments |
| tests/macro_expansion/test_macro_expansion.py | Updated multiple test methods to use new Preprocessor constructor |
codebasin/preprocessor.py
Outdated
Comment on lines
1681
to
1691
| platform_name: str | None = None, | ||
| rootdir: str | os.PathLike[str] | None = None, | ||
| include_paths: list[str | os.PathLike[str]] | None = None, | ||
| defines: list[str] | None = None, | ||
| ) -> None: | ||
| if platform_name is None: | ||
| self.platform_name = None | ||
| elif not isinstance(platform_name, str): | ||
| raise TypeError("'platform_name' must be a string.") | ||
| else: | ||
| self.platform_name = platform_name |
There was a problem hiding this comment.
The constructor allows platform_name to be None, but the logic in associate method assumes it has a value when calling association[node].add(preprocessor.platform_name). This could lead to runtime errors if None is passed.
Suggested change
| platform_name: str | None = None, | |
| rootdir: str | os.PathLike[str] | None = None, | |
| include_paths: list[str | os.PathLike[str]] | None = None, | |
| defines: list[str] | None = None, | |
| ) -> None: | |
| if platform_name is None: | |
| self.platform_name = None | |
| elif not isinstance(platform_name, str): | |
| raise TypeError("'platform_name' must be a string.") | |
| else: | |
| self.platform_name = platform_name | |
| platform_name: str, | |
| rootdir: str | os.PathLike[str] | None = None, | |
| include_paths: list[str | os.PathLike[str]] | None = None, | |
| defines: list[str] | None = None, | |
| ) -> None: | |
| if not isinstance(platform_name, str): | |
| raise TypeError("'platform_name' must be a string.") | |
| self.platform_name = platform_name |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I agree with the comment but not the suggestion.
I've added a check for None inside associate, in 0214a3f.
The root directory was used by a previous implementation of find_include_file, but is no longer required. Signed-off-by: John Pennycook <[email protected]>
The new Preprocessor class allows platform_name to be None, to enable uses that invoke the preprocessor without also building a setmap. Attempting to build a setmap when the preprocessor does not have an associated platform_name should result in an error. Signed-off-by: John Pennycook <[email protected]>
Signed-off-by: John Pennycook <[email protected]>
The tests highlighted a bug in the implementation where an iterable (like a string) was not detected as an invalid argument and did not throw a TypeError. Signed-off-by: John Pennycook <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issues
Platformclass into preprocessor.py #198.Proposed changes
PlatformtoPreprocessor, sincePlatformis not consistent with how we use the name "platform" elsewhere.PreprocessorAPI to improve readability.