-
Notifications
You must be signed in to change notification settings - Fork 620
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
Pascal: Add parsing of function/proc signatures #3205
Pascal: Add parsing of function/proc signatures #3205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3205 +/- ##
==========================================
+ Coverage 84.84% 84.94% +0.10%
==========================================
Files 205 206 +1
Lines 48369 49030 +661
==========================================
+ Hits 41040 41650 +610
- Misses 7329 7380 +51
Continue to review full report at Codecov.
|
98c81b0
to
8c02ce0
Compare
If you are thinking about your hoidays:-), I would like you to see #2241, too. |
Ha, nice try. |
@eht16, I see. |
parsers/pascal.c
Outdated
} | ||
|
||
*end = '\0'; | ||
*arglist = strdup(start); |
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.
My understanding:
To do *end = '\0'
, you have to do c = strdup(buf)
.
To do strdup(start)
, you have to do *end = '\0'
, terminating the C string.
I think you can reduce strdup()
with eStrndup(const char* str, size_t len);
declared in main/routines.h
.
With it, you can make a new C string without doing *end = '\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.
Thanks for the hint, did it and then rewrote it with your vString
suggestion below :D.
parsers/pascal.c
Outdated
*end = '\0'; | ||
*arglist = strdup(start); | ||
|
||
eFree(c); |
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 eStrndup
helps you expectedly, you can remove this eFree()
.
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.
Fortunately, that code vanished by using vString
.
parsers/pascal.c
Outdated
@@ -74,6 +80,68 @@ static bool tail (const char *cp) | |||
return result; | |||
} | |||
|
|||
static void parseArglist(const char *buf, char **arglist, char **vartype) |
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.
Using vString() for storing arglist and vartype is an alternative approach.
In a micro view, an operation on vString tasks higher const than using char *
directly.
However, in a macro view, vString is not so bad. Using char *, you must call strdup twice in every calling parseArglist.
vString can be reused once it is new'ed in findPascalTags.
A vString can clear its internal buffer keeping char * with vStringClear.
Though, it is cleared, the buffer is not deallocated.
Writing English is hard for me. Let's show me in pseudo-code:
/* Your code using raw C string */
static void parseArglist(const char *buf, char **arglist, char **vartype)
{
*arglist = strdup();
*vartype = strdup();
}
static void findPascalTags (void)
{
char *arglist = NULL;
char *vartype = NULL;
// ...
loop() {
createPascalTag();
// ...
free (arglist);
free (vartype);
parseArglist(, &arglist, &vartype);
}
free (arglist);
free (vartype);
}
/* What we can do with vString */
static void parseArglist(const char *buf, vString *arglist, vString *vartype)
{
char c, *str;
/* ... */
vStringPut(arglist, c);
/* or */ vStringCatS(arglist, str);
/* or */ vStringNCatS(arglist, str, len);
vStringPut(vartype, c);
/* or */ vStringCatS(vartype, str);
/* or */ vStringNCatS(vartype, str, len);
}
static void findPascalTags (void)
{
vString *arglist = vStringNew();
vString *vartype = vStringNew();
// ...
loop() {
createPascalTag();
// ...
vStringClear (arglist);
vStringClear (vartype);
parseArglist(, arglist, vartype);
}
vStringDelete (arglist);
vStringDelete (vartype);
}
Of course, I don't care about the performance of this level.
However, vString helps me understand the logic.
This comment is not for "must-fix". Your code is already works fine. So this is a just 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.
Great idea and thanks for the detailed explanation.
I rewrote the code to use vString
(back in 2009 when I wrote the code initially I wasn't so much aware of the vString
helper and didn't touch the since then).
It's simpler now and we don't need any more string copying than what is done implicitly by vStringCat*()
.
NOTE for myself: following semi-fuzz tests are passed.
|
The |
parsers/pascal.c
Outdated
parseArglist((const char*) cp, &arglist, (kind == K_FUNCTION) ? &vartype : NULL); | ||
vStringClear (arglist); | ||
vStringClear (vartype); | ||
parseArglist((const char*) cp, arglist, (kind == K_FUNCTION) ? vartype : NULL); |
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.
A minor issue. The line 282 and the line 283 are not aligned.
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.
Line 282 uses only tag characters.
However, the line 283 uses tabs and spaces mixed way.
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.
Oops, will fix.
Another thing I noted (and probably was introduced by me in 2009), there are some inconsistencies in using a space between function name and the opening brace of the argument list. Should I fix them as well, to consistently use a space between?
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.
Should I fix them as well, to consistently use a space between?
Thank you for the offering. If possible, could you fix them in ANOTHER commit?
In a commit, let's do one thing.
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.
Ok, the wrong indentation is fixed.
Did you mean another commit in this PR to fix the argument list space problem or a separate PR?
I don't mind.
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 also don't mind. However, you already submitted a good commit that aligns with the goal of this pull request.
So I will merge this.
6205ac2
to
bd60584
Compare
Thank you. |
Parse the signatures and return type for functions and procedures in Pascal.
This features is ported from the Geany Pascal parser where we use this code already for many years and it might be useful for others too.