-
Notifications
You must be signed in to change notification settings - Fork 6
Demo of clang AST #17
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
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 69283ee.
|
Any feedback? |
|
I'll update this if/when we have a merge of PrairieLearn/PrairieLearn#11913. |
jonatanschroeder
left a comment
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.
This is a pretty interesting project, here are a few comments to improve the code.
| decl = find_const_double(translation_unit.cursor) | ||
| if decl is None: | ||
| self.add_test_result("const double not found", points=0, max_points=1) | ||
| return | ||
| self.add_test_result("const double found") | ||
| if not var_name_is_valid(decl): | ||
| self.add_test_result("variable name is not using the capitalization expected for a constant variable", points=0, max_points=1) | ||
| return | ||
| self.add_test_result("variable name is valid") | ||
| initializer = find_double_initializer(decl) | ||
| if initializer is None: | ||
| self.add_test_result("variable does not have a double initializer", points=0, max_points=1) | ||
| return | ||
| self.add_test_result("variable has a double initializer") | ||
| if not initializer_is_scientific_notation(initializer): | ||
| self.add_test_result("initializer is not in scientific notation", points=0, max_points=1) | ||
| return | ||
| self.add_test_result("initializer is in scientific notation") |
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'm not a huge fan of conditional tests, especially since they mess up with the total points and don't give students a sense of what other tests are coming up. Also, this method allows some tests to run together (e.g., show both the initializer test and the var name test if either fails).
| decl = find_const_double(translation_unit.cursor) | |
| if decl is None: | |
| self.add_test_result("const double not found", points=0, max_points=1) | |
| return | |
| self.add_test_result("const double found") | |
| if not var_name_is_valid(decl): | |
| self.add_test_result("variable name is not using the capitalization expected for a constant variable", points=0, max_points=1) | |
| return | |
| self.add_test_result("variable name is valid") | |
| initializer = find_double_initializer(decl) | |
| if initializer is None: | |
| self.add_test_result("variable does not have a double initializer", points=0, max_points=1) | |
| return | |
| self.add_test_result("variable has a double initializer") | |
| if not initializer_is_scientific_notation(initializer): | |
| self.add_test_result("initializer is not in scientific notation", points=0, max_points=1) | |
| return | |
| self.add_test_result("initializer is in scientific notation") | |
| decl = find_const_double(translation_unit.cursor) | |
| self.add_test_result("const double found", points=(decl is not None)) | |
| var_name_result = decl is not None and var_name_is_valid(decl) | |
| self.add_test_result("variable name is valid", | |
| points=1 if var_name_result else 0, | |
| output=None if var_name_result else "variable name is not using the capitalization expected for a constant variable" | |
| ) | |
| self.add_test_result("variable has a double initializer", | |
| points=1 if decl is not None and find_double_initializer(decl) else 0 | |
| ) | |
| self.add_test_result("initializer is in scientific notation", | |
| points=1 if decl is not None and initializer_is_scientific_notation(initializer) else 0 | |
| ) |
| "./student", | ||
| input="65.00\n", | ||
| exp_output="55.25", | ||
| must_match_all_outputs="partial", |
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.
This is not needed when there is a single output.
| exp_output="40.8", | ||
| must_match_all_outputs="partial", | ||
| ) | ||
| self.add_test_result("code runs correctly") |
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.
If the ast tests pass, this will always return true even if the code doesn't print anything valid.
| import cgrader | ||
| import clang.cindex | ||
|
|
||
| clang.cindex.Config.set_library_path("/usr/lib/") |
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.
| "enabled": true, | ||
| "image": "prairielearn/grader-c", | ||
| "entrypoint": "/grade/tests/test.sh", | ||
| "serverFilesCourse": ["clang/"] |
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.
This line and the serverFilesCourse content become unnecessary once PrairieLearn/PrairieLearn#11913 is handled. Also, the entrypoint can become the Python file again if the path does not need to be updated.
|
@jonatanschroeder, Is this PR of much value after PrairieLearn/PrairieLearn#11913? I think the factorial example we added there is probably a better demo (especially after your rewrite!). |
@jgfoster it's your call. I believe it still has merit, focusing on items that are not covered by the factorial question in the PL official course, such as the fact that you're using C++ instead of C, and that you are covering other kinds of AST evaluations. That said, I think retrieving some of the changes from that PR and incorporating them here would probably be a good idea (though I don't think you need to migrate to |
# Conflicts: # questions/Scratch/block_position_1/info.json # questions/Scratch/block_position_2/info.json # questions/Scratch/category_for_conditionals/info.json # questions/Scratch/category_for_input/info.json # questions/Scratch/category_for_loops/info.json # questions/Scratch/category_for_say/info.json # questions/Scratch/loop_choice/info.json # questions/Scratch/project_1/info.json # questions/Scratch/project_1/server.py # questions/Scratch/project_2/info.json # questions/Scratch/project_2/server.py # questions/Scratch/project_3/info.json # questions/Scratch/project_3/server.py # questions/Scratch/project_4/info.json # questions/Scratch/project_4/server.py # questions/Scratch/project_5/info.json # questions/Scratch/project_5/server.py # questions/Scratch/project_6/info.json # questions/Scratch/project_6/server.py # questions/Scratch/project_7/info.json # questions/Scratch/project_7/server.py # questions/Scratch/project_8/info.json # questions/Scratch/project_8/server.py # questions/Scratch/project_9/info.json # questions/Scratch/project_9/server.py # questions/Scratch/scratch_editor/info.json # questions/Scratch/stage_position/info.json # questions/Scratch/what_is_Boolean/info.json # questions/Scratch/what_is_a_costume/info.json # questions/Scratch/what_is_a_function/info.json # questions/Scratch/what_is_a_global/info.json # questions/Scratch/what_is_a_handler/info.json # questions/Scratch/what_is_a_local/info.json # questions/Scratch/what_is_a_script/info.json # questions/Scratch/what_is_a_sprite/info.json # questions/Scratch/what_is_a_value/info.json # questions/Scratch/what_is_a_variable/info.json # questions/Scratch/what_is_abstraction/info.json # questions/Scratch/what_is_an_event/info.json # questions/Scratch/what_is_an_input/info.json # questions/Scratch/what_is_an_object/info.json # questions/Scratch/what_is_broadcasting/info.json # questions/Scratch/what_is_join/info.json # questions/Scratch/what_is_round/info.json
| "credit": 100 | ||
| } | ||
| ], | ||
| "text": "This assessment follows CS50's Introduction to Programming with Scratch, OpenCouraseWare from Harvard University (https://cs50.harvard.edu/scratch/2024/).", |
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.
| "text": "This assessment follows CS50's Introduction to Programming with Scratch, OpenCouraseWare from Harvard University (https://cs50.harvard.edu/scratch/2024/).", | |
| "text": "This assessment follows CS50's Introduction to Programming with Scratch, OpenCourseWare from Harvard University (https://cs50.harvard.edu/scratch/2024/).", |
One char typo
|
|
||
| cout << "Enter an item price: $"; | ||
| cin >> price; | ||
| cout << fixed << setprecision(2) << price * (1.0 - DISCOUNT) << endl; |
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.
The example includes a text, the solution should probably include it too, even if it's not enforced in the autograder.
| cout << fixed << setprecision(2) << price * (1.0 - DISCOUNT) << endl; | |
| cout << "The sale price is " << fixed << setprecision(2) << price * (1.0 - DISCOUNT) << endl; |
| ) | ||
|
|
||
| self.add_test_result("initializer is in scientific notation", | ||
| points=1 if decl is not None and initializer_is_scientific_notation(initializer) else 0 |
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.
| points=1 if decl is not None and initializer_is_scientific_notation(initializer) else 0 | |
| points=1 if initializer is not None and initializer_is_scientific_notation(initializer) else 0 |
Alternative: change the function to accept a None and return False.
I figured out how to parse a C++ program!