Parse C #2
Parse C #2
Conversation
We found a parsing library last week. There is a test file showing that it works, but need to also test it on our own files. Will do that as soon as possible, sorry for the late commit
…into SimpleApp
This reverts commit e2ccaaf.
There was a problem hiding this comment.
Looked at Parse.java and ParseTest.java - both are on the right track and seem functionally fine, though there's some style comments (general note: this is always expected from a code review, and style is important for code readability and maintainability in team projects) and some larger points we can discuss.
Additionally, there seems to be a lot of other irrelevant files - can you do a first pass on purging those? Good practice is to keep the repository clean and pull requests focused on the feature at hand. Pulling in external code directly may also have copyright implications.
| if (child instanceof CPPASTSimpleDeclaration) { | ||
|
|
||
| /**Setup*/ | ||
| CPPASTSimpleDeclaration declaration = (CPPASTSimpleDeclaration) child; |
There was a problem hiding this comment.
Style nit: it may be helpful to add an example here of what is being extracted, eg uint8_t PEDAL_POS = 8; here, at least if these are different (I'm not sure)
| CPPASTEqualsInitializer init = (CPPASTEqualsInitializer) declarator.getInitializer(); | ||
|
|
||
| /**Get what is needed: Value, Name, bigType & smallType*/ | ||
| CPPASTLiteralExpression value = (CPPASTLiteralExpression) init.getInitializerClause(); //get value |
There was a problem hiding this comment.
Style comment (... about comments!): comments should generally not be redundant with what can be understood by reading the code - in this case, I would say that these short comments are unnecessary, since you did a good job of writing self-documenting code.
In general, comments are often useful for providing examples, summarizing behavior, or documenting tricky behavior / oddities.
| /**Get what is needed: Value, Name, bigType & smallType*/ | ||
| CPPASTLiteralExpression value = (CPPASTLiteralExpression) init.getInitializerClause(); //get value | ||
| CPPASTName name = (CPPASTName) declarator.getName(); //get name | ||
| IToken bigType = specifier.getSyntax(); //get big type (iToken) |
There was a problem hiding this comment.
What do you mean big type and small type? From the below, if you mean bigType for things like const, it's more formally called a type qualifier, while uint32_t is the actual type. Using (and finding) standardized terminology is helpful for writing clear code.
There might also be other ways to simplify this distinction - we should talk about it.
| } | ||
|
|
||
| /**Adds a new hashmap for Large Types if bType does not exist*/ | ||
| private void checkBigTypeExists(IToken bType) { |
There was a problem hiding this comment.
The function name (check...) implies this should return something, but it doesn't? Perhaps you mean "recordBigType" or "addBigType"?
We might also think of ways to restructure the data structures to simplify this - we should discuss that.
Similar for below.
| public class Parse { | ||
|
|
||
| private IASTTranslationUnit translationUnit; //current translation unit node | ||
| public HashMap<IToken, HashMap> bigTypes; |
There was a problem hiding this comment.
Did you know you can (I think) also add type parameters for the inner hash map, eg HashMap<IToken, HashMap<..., ...>>? Comprehensively documenting types is very useful.
| } | ||
|
|
||
| /**Initializes the translation unit setup*/ | ||
| public static IASTTranslationUnit getIASTTranslationUnit(char[] code) throws Exception { |
| public void showNameValue() { | ||
| iterateBigTypes(); | ||
| } | ||
| /**Iterate through all big types eg. const*/ |
There was a problem hiding this comment.
In production code, I generally prefer not to leave in debugging code. Obviously a bit of season-to-taste (especially if you're building data structures utilities).
If you directly print the HashMap, will it do the right thing and print the contents?
| String value1 = "0x16"; | ||
| String value2 = "0x32"; | ||
| String value3 = "0x64"; | ||
|
|
||
| String n1 = "CAN_ID"; | ||
| String n2 = "CAN_ID2"; | ||
| String n3 = "CAN_ID3"; |
There was a problem hiding this comment.
Feel free to inline these into the below lines for conciseness.
Sometimes, declaring values separately can be a stylistic benefit, by giving them a name, but this test code would be clear with the "magic numbers" (or magic strings in this case) directly inline.
| CPPASTName name2 = new CPPASTName(n2.toCharArray()); | ||
| CPPASTName name3 = new CPPASTName(n3.toCharArray()); | ||
|
|
||
| String litExpr1 = test.get(name1).getRawSignature(); |
There was a problem hiding this comment.
Would it make sense to have get take a string as its argument instead of this CPPASTName?
|
|
||
| @Test | ||
| public void checkLitExprdiverse() throws Exception { | ||
| String code = "const uint16_t CAN_ID = 0x16; \n" + |
There was a problem hiding this comment.
Similar comments here - I believe inlining should make this test a lot more readable.
|
I'll also acknowledge that part of this is probably a bit chaotic (with regards to file organization, not the particular code written) since I didn't set up a template project so it's less clear what should go where. We should also work through that, which might also be related to #1. |
This is a program that parses C code.
Input: string of C code
eg. Parse test = new Parse(code);
Output: Object that holds a repository of all C variables and its values.