search

dlbeer / quirc

QR decoder library
Other
882 stars 286 forks source link

Makefile: Use = for CFLAGS, instead of ?= #95

Closed yamt closed 3 years ago

yamt commented 3 years ago

Some make (eg. BSD make I installed with "brew install bsdmake") have the default CFLAGS set. Using ?= here means to use the default. I guess it isn't the intention of this Makefile.

Also, update an example to override CFLAGS accordingly.

yamt commented 3 years ago

?= is intended here and a feature, allowing to set QUIRC_MAX_REGIONS for example (see the README).

From the CFLAGS we set, I'm personally Ok with make and/or the system overriding -Wall and -03. On the other hand,-fPIC is not optional. So I could see something like this:

CFLAGS ?= -O3 -Wall
CFLAGS += -fPIC

you can override variables defined with =.

yamt commented 3 years ago

@kaworu i guess i now understand what you meant. i updated the patch accordingly.

kaworu commented 3 years ago

Out of curiosity, what are the default CFLAGS defined by bmake installed by brew on your system?

yamt commented 3 years ago

-Os -pipe. it's from the default sys.mk. it can be disabled with bsdmake -r.

kaworu commented 3 years ago

Typically, I believe -Os -pipe is reasonable to replace our default -O3 -Wall. I don't know a build system that doesn't honor the environment CFLAGS or CC. The env might have some strong opinion on CFLAGS like -fstack-protector-strong or cross compiling overriding CC as well etc.

Again, the only issue I see is about the -fPIC which the patch doesn't address as-is.

yamt commented 3 years ago

Typically, I believe -Os -pipe is reasonable to replace our default -O3 -Wall. I don't know a build system that doesn't honor the environment CFLAGS or CC. The env might have some strong opinion on CFLAGS like -fstack-protector-strong or cross compiling overriding CC as well etc.

when you say the environment, you specifically mean the environment variables, and you are not happy with "make CFLAGS=${CFLAGS}", right?

Again, the only issue I see is about the -fPIC which the patch doesn't address as-is.

i want to make it use -Wall by default too. also, i want to keep it overridable by user.

yamt commented 3 years ago

also, IMO, -fPIC should be overridable as well because it's very compiler/platform dependent.

yamt commented 3 years ago

after thinking a bit, i tend to think documentation is enough. https://github.com/dlbeer/quirc/pull/101