Refactor helper functions around attributes #3291 #3293
Conversation
| static std::mutex mtx; | ||
| static BuiltinAttributeMappings* instance = nullptr; | ||
|
|
||
| if (instance == nullptr) { | ||
| std::lock_guard<std::mutex> lock(mtx); | ||
| if (instance == nullptr) { | ||
| instance = new BuiltinAttributeMappings(); | ||
| } | ||
| } |
There was a problem hiding this comment.
why did you add a mutex here? we don't need one, as gccrs is single threaded and no two concurrent threads will try and access those attribute mappings.
| @@ -105,39 +109,77 @@ BuiltinAttributeMappings::BuiltinAttributeMappings () | |||
| mappings.insert ({def.name, def}); | |||
| } | |||
| } | |||
| class Attributes { | |||
There was a problem hiding this comment.
There is already an Attributes class in rust-attributes.h, which you should add functions to.
| // Use the centralized methods from the Attributes class | ||
| if (Attributes::has_attribute(crate.get_inner_attrs(), "example_attribute")) { | ||
| std::string value = Attributes::get_attribute_value(crate.get_inner_attrs(), "example_attribute"); | ||
| // Now you can use the 'value' as needed |
There was a problem hiding this comment.
Why are we checking if the crate has an "example attribute" attribute here? This seems like it shouldn't be here. The purpose of AttributeChecker::go is simply to provide a public entry-point for the visitor
| // Use the centralized methods from the Attributes class | ||
| if (Attributes::has_attribute(item->attributes, "example_attribute")) { | ||
| auto value = Attributes::get_attribute_value(item->attributes, "example_attribute"); | ||
| // Now you can use the 'value' as needed |
There was a problem hiding this comment.
Same here. We don't want to check for "example_attribute"
| // Builtin attributes always have a single segment. This avoids us creating | ||
| // strings all over the place and performing a linear search in the builtins | ||
| // map | ||
| if (segments.size () != 1) | ||
| return false; | ||
| // Check if it's a single-segment path |
There was a problem hiding this comment.
why did you remove the comment?
| static bool is_proc_macro_type(const AST::Attribute &attribute) | ||
| { | ||
| BuiltinAttrDefinition result; | ||
| if (!is_builtin (attribute, result)) | ||
| return false; | ||
|
|
||
| auto name = result.name; | ||
| return name == Attrs::PROC_MACRO || name == Attrs::PROC_MACRO_DERIVE | ||
| || name == Attrs::PROC_MACRO_ATTRIBUTE; | ||
| return Attributes::is_proc_macro_type(attribute); | ||
| } |
There was a problem hiding this comment.
This is good but we'd want to remove the function all together and instead call Attributes::is_proc_macro_type throughout the file.
|
@Harishankar14 your PR message mentions the creation of the it's no problem to ask an LLM for help with understanding a codebase, but using one to generate code for a project you don't understand is not great. I would much rather spend time chatting with you on the issue and replying to your questions, and helping you get started on the project. if you need I'm happy to help by chatting on our Zulip or here on github. |
|
Hii,
Yeah I really messed it up, I did use LLM inorder to understand the
codebase,and yeah some of the parts, I did try my code, and verify with an
LLM, I forgot to recheck my version of code which I had written and the
code which was given my an LLM,I got confused,and was in a hurry ,so
extremely sorry for this cause, will try to rework !!
…On Thu, 5 Dec, 2024, 4:24 pm CohenArthur, ***@***.***> wrote:
@Harishankar14 <https://github.com/Harishankar14> your PR message
mentions the creation of the Attributes class, which already exists in
the codebase. did you use an LLM to write the code as well? It does not
feel like you really understand what you modified, or why you modified it.
A lot of the code you changed didn't need to be changed, and you missed
multiple uses of the "old" attribute system which would have needed to be
refactored as well. You only modified one file, which indicates you didn't
spend a lot of time searching in the code base for what to replace with
your new attribute system.
it's no problem to ask an LLM for help with understanding a codebase, but
using one to generate code for a project you don't understand is not great.
I would much rather spend time chatting with you on the issue and replying
to your questions, and helping you get started on the project. if you need
I'm happy to help by chatting on our Zulip or here on github.
—
Reply to this email directly, view it on GitHub
<#3293 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVE3LP5V3MWFTFMSJDSUVGD2EAWFHAVCNFSM6AAAAABTB72DEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJZHE3DMMZSHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
In the original codebase, attribute checks were scattered across different functions, leading to a lot of repetition and scattered logic. The goal was to centralize the functionality related to attribute checking and retrieval into a single, reusable Attributes class. This would help improve maintainability, reduce redundancy, and improve the clarity of the codebase.
Changes Made:
Created the Attributes Class:
Introduced a new static utility class Attributes to centralize common operations on attributes such as checking for the existence of an attribute and retrieving its value.
The class includes the following functions:
has_attribute: Checks if an attribute with a specified name exists in a list of attributes.
get_attribute_value: Retrieves the value of an attribute by its name, returning an empty string if the attribute is not found.
is_proc_macro_type: Determines if an attribute corresponds to a "proc-macro" type, which is used to check attributes that are related to procedural macros.
get_builtin_type: A new helper function that maps attribute names to an enumeration (BuiltinAttributeType). This centralizes the mapping logic, making it easier to extend in the future.
Refactored the AttributeChecker Class:
Updated the check_attribute method to use the new Attributes class instead of direct attribute checks.
Instead of using multiple checks for each attribute, the check_attribute function now uses the Attributes::get_builtin_type to map attribute names to their corresponding type. This enables more modular and maintainable handling of attribute types.
Used the centralized attribute checking functions (has_attribute, get_attribute_value, is_proc_macro_type) to streamline the code and remove redundancy in the attribute checks.
Simplified Attribute Checks in Various Functions:
Replaced repetitive checks for specific attribute types (like PROC_MACRO, DOC, etc.) with calls to Attributes::is_proc_macro_type and Attributes::get_builtin_type. This reduces code duplication and improves the clarity of the logic.
Simplified the check_proc_macro_non_function and check_proc_macro_non_root functions by using Attributes::is_proc_macro_type to handle the checking of proc-macro attributes centrally.
Improved Error Handling for Invalid Attributes:
In functions like check_proc_macro_non_function and check_proc_macro_non_root, used the new Attributes::is_proc_macro_type function to check for proc-macro types and handle errors, making the code cleaner and more maintainable.
The error handling now leverages the rust_error_at function to print clear error messages when attributes are used incorrectly (e.g., proc-macro attributes used on non-function items or outside the crate root).
Enum for Builtin Attribute Types:
Introduced the BuiltinAttributeType enum to represent known attribute types (e.g., DOC, PROC_MACRO). This provides a more structured way to manage attribute types and makes it easier to add new types in the future.
The mapping of attribute names to types is now centralized in the get_builtin_type function, avoiding repetitive string-based checks