Skip to content
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

add -u flag, error if variable undefined #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

birchb1024
Copy link

No description provided.

@xyb3rt
Copy link
Collaborator

xyb3rt commented Jun 11, 2023

What is your opinion on this, @rakitzis? Can you please add some tests to trip.rc for this, @birchb1024?

return NULL; /* not found */
if (look == NULL) {
if (strict && dashewe)
rc_error(get_message("Undefined variable '%s'\n", name));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inconsistent capitalization

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer "undefined ?

@@ -52,14 +52,21 @@ extern bool varassign_string(char *extdef) {
return TRUE;
}

char* get_message(char *format, char *msg) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer char *-style decl syntax

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole helper function actually is unneeded -- and allocating error messages with malloc will leak memory.

Incidentally, trip.rc probably doesn't verify leak-free behavior but at one time I ran rc through valgrind to catch leaks. I wonder if the state of the art of leak detection for C has advanced to the point where we could run a tool in-line for pull requests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a while since I ran this, but AFAICR trip.rc only succeeds with this flag OFF. Because there are undefined variables being used in there 😃 . Notwithstanding I have been using this branch as my daily drive. I will have a look at this again.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trip.rc can exercise features selectively. It's not all-or-nothing.

@@ -1,5 +1,5 @@
/* var.c: provide "public" functions for adding and removing variables from the symbol table */

#include <stdio.h>
Copy link
Owner

@rakitzis rakitzis Jun 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer the nalloc helper, do not include stdio or use malloc here. nalloc is scoped to command-lifetime allocations.

@rakitzis
Copy link
Owner

Seems like a useful feature, but the change could be harmonized with the [admittedly idiosyncratic] coding style.

@rakitzis
Copy link
Owner

And yes, it could use some test coverage in trip.rc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants