search

carp-lang / Carp

A statically typed lisp, without a GC, for real-time applications.
Apache License 2.0
5.52k stars 173 forks source link

refactor: change project flag configuration behavior #1451

Open scolsen opened 1 year ago

scolsen commented 1 year ago

This PR changes the behavior of the project flag setting dynamic functions. Instead of appending a single string to the flagset, they now take a list of strings, which serve as the new flagset:

(Project.get-config "cflag")
=> (...a bunch of default platform flags; "-wall" ...)
(Project.config "cflag" '("-my" "-new" "-flags"))
(Project.get-config "cflag")
=> ("-my" "-new" "-flags")

In order to append flags, users now need to cons to a config-get:

(Project.config "cflag" (cons "-new" (Project.get-config "cflag")))

This is a breaking change, as passing a single string is now an error.

I considered overloading the behavior as an alternative so that we wouldn't break existing callers, but I feel this would ultimately lead to unintuitive behavior (calling with a single string appends while calling with a list of strings overrides ... users might wonder why the list doesn't append all the strings in the list instead).

This PR also fixes a bug w/ fetching project C flags and provides helper macros for appending flags to existing project flag sets given the new behavior.

scolsen commented 1 year ago

Ok, so this PR gets our failing *nix builds green again by bypassing -Wunknown-warning-option, but it now seems that there's yet another, totally unrelated Macos SDK image error complaining about the use of sprintf....ugh

scolsen commented 1 year ago

I'm wondering if we should just drop -Wall for our tests at this point in favor of an explicitly set list of reasonable warnings we want to make sure our emitted C complies with.

hellerve commented 1 year ago

Possibly a good idea, but maybe we should also just remove usages of sprintf. I’ve done that in #1453 for your convenience :)

scolsen commented 1 year ago

Possibly a good idea, but maybe we should also just remove usages of sprintf. I’ve done that in #1453 for your convenience :)

Even better!

eriksvedang commented 1 year ago

Does this need to pull in master for the tests to pass?

eriksvedang commented 1 year ago

@scolsen Should we try to get this merged?