-
Notifications
You must be signed in to change notification settings - Fork 7
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
Create a class registration file #234
Conversation
denizumuteser
commented
Nov 8, 2022
- Updated instantiation generation script to read from common JSON file.
- Added a common JSON file and moved all hardcoded class templates there.
for each_class in class_instantiation_list: | ||
#print(each_class) | ||
#setting current filename | ||
if current_filename != each_class.get("filename"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this mechanism requires the writers into class_instantiation_list.json
to make sure they list out all the classes of a file sequentially It is a reasonable thing to ask of the developers, but if they don't do it accidentally, it will lead to weird linking errors.
I think a better approach is to store a dictionary of opened files and only reset a file the first time it is encountered.
|
||
#choosing folder | ||
current_folder = each_class.get("folder") | ||
if each_class.get("ifdef") == "USE_CUDA": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better, more generic, approach here is to repurpose the folder
field in the JSON to actually store the name of the sub-folder to store the init files (the folder inside output_folder
). So if it's null
, store it in the default folder, and if it's not null
, then store it in os.path.join(output_folder, each_class.get("folder"))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just address the comments I left and I think it's good to go.
One thing we lose with this approach, though, is the ability to dynamically generate some of these template strings (like how loops were being used to generate template strings for the CsrCUDACsrConditionalFunction
function and its likes). However, this JSON will hopefully be used to also generate other parts of the code (docs specifically) so I think the pros outweigh the cons.