rmyorston / pdpmake

Public domain POSIX make
https://frippery.org/make
Other
107 stars 11 forks source link

Implement pattern macro expansions #5

Closed illiliti closed 2 years ago

illiliti commented 2 years ago
SRC = source.c
OBJ = $(SRC:%.c=build/%.o)

print_objs:
    @echo $(OBJ)

See also: https://www.austingroupbugs.net/view.php?id=519


TODO:

rmyorston commented 2 years ago

Sorry, my mind is befuddled after reading the Austin Group defect report.

Why find[0] != '\0'?

if (!posix && find[0] != '\0' && (find_perc = strchr(find, '%'))) {

(So befuddled that I was using vi editing commands in the comment box. Must. Sleep. Now.)

illiliti commented 2 years ago

Oops, that was a mistake i think. Will fix.

rmyorston commented 2 years ago

The percent in the find string can match zero characters, so op%os matches opos. Currently the patch doesn't handle that. With this makefile:

WORD = osop
target:;@echo $(WORD:os%op=ns%np)

GNU make returns nsnp while we return osop.

The length of the word can be one character less than the length of the find string and still match. This patch to your commit adjusts the length tests to suit: input.txt

rmyorston commented 2 years ago

A couple of other minor things:

illiliti commented 2 years ago

GNU make returns nsnp while we return osop.

Nice catch! Thanks for testing. I applied your patch and added test to cover this behavior.

if we don't have faith in our arithmetic we should check the return value of snprintf

I came up with a different approach. Instead of calculating len manually, call snprintf(NULL, 0, ...) to get len, allocate and then call sprintf(newword, ...). That's how asprintf work by the way.

It would be good if the commit message mentioned how multiple percent characters in the replacement string are handled. And that the chosen behaviour matches GNU make.

Did my best.

E5ten commented 2 years ago

GNU make returns nsnp while we return osop.

Nice catch! Thanks for testing. I applied your patch and added test to cover this behavior.

if we don't have faith in our arithmetic we should check the return value of snprintf

I came up with a different approach. Instead of calculating len manually, call snprintf(NULL, 0, ...) to get len, allocate and then call sprintf(newword, ...). That's how asprintf work by the way.

I'm not sure if this is a better method, because it's certainly going to be slower than using already available info to calculate length when compared to an entire dry-run printf call. imo the previous code was best, because the arithmetic should be correct. I think the best way would be to revert to that, but maybe add an assert() that checks the return value of snprintf? Because a failed sprintf/snprintf wouldn't be a normal error, but something incorrect in the code, so that seems like the best way to verify correctness without sacrificing performance due to 2 printf calls, or risking harm through using sprintf should the arithmetic turn out to somehow be wrong.

It would be good if the commit message mentioned how multiple percent characters in the replacement string are handled. And that the chosen behaviour matches GNU make.

Did my best.

illiliti commented 2 years ago

Reverted the change and added assert check as per @E5ten suggestion.

rmyorston commented 2 years ago

I've reworked the first patch somewhat: 0001-Implement-pattern-macro-expansions.txt. It still passes the tests, though I haven't done any more testing than that.

I was concerned that splitting of the find/replace patterns at the <percent> happened inside the loop over each word in modify_words(). This is clearly inefficient: the patterns are going to be the same for every word. The splitting has been moved up into expand_macros(), to the same place where the simple suffix replacements were already being handled.

As well as describing pattern macro expansions the defect report also says:

In both macro expansion forms, any macro expansions on the right hand side of the colon shall be recursively expanded before further examination. If this results in more than one <equals-sign> after the <colon>, the first one shall be considered to be the separator.

I think this is a clarification of how macro expansions to the right of the colon are supposed to work rather than a new feature. The existing code splits at the <equals-sign> before performing macro expansion. Clearly this isn't what was intended and this patch fixes the code.

illiliti commented 2 years ago

Nice! Applied

rmyorston commented 2 years ago

Patch applied. I've edited the commit message for the first commit but apart from that it's unaltered.

Thanks for your patience. I haven't been able to give this my full attention as I've had a couple of time-critical projects on the go over the last week.

illiliti commented 2 years ago

Thanks! No worries, i understand.