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 this_file, this_counter, GNUremake #116

Open
wants to merge 8 commits into
base: remake-4-3
Choose a base branch
from
Open

Conversation

rocky
Copy link
Owner

@rocky rocky commented Jan 9, 2021

@bstarynk this addresses the build failure and has a test for GNUremakefile

However I don't see that the this_ functions work.

For example for this Makefile

B = c d $(this_file) $(this_line) $(this_counter)

x:
	@echo $(B)

I get:

$ ../make -x -f this.Makefile
Reading makefiles...
Updating makefiles...
Updating goal targets...
 File 'x' does not exist.
Must remake target 'x'.
this.Makefile:5: target 'x' does not exist
##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
echo c d   
##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
c d
Successfully remade target file 'x'.

If the this functions work for you, maybe you can give an example simiilar to the above?

@boretom
Copy link
Collaborator

boretom commented Jan 11, 2021

Hey, long time not chat :)

I had a first look at it and the func-call-with-no-argument handling in function.c seems to need work. In the Makefile using something like C = c d $(this_file fluffy) works but C = c d $(this_file) doesn't.

In the function table a function with a max of 0 args can't be specified since max=0 means no maximum. A workaround could be change the syntax, like $(this file). That would allow for a shorter form if file, line and counter is needed, e.g. $(this file counter) but does make argument checking necessary.

Without any args expand_builtin_function seems not to be called at all in the above case.

In the next few days I'll have a closer look.

/Thomas

Edit: Remove "there's no check for max # of arguments", the first paragraph was after I did some changing, not true for the unmodfied commit

Copy link
Collaborator

@boretom boretom left a comment

Choose a reason for hiding this comment

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

Hey @rocky

Addition to the above: I did some more investigation and the lookup_function method doesn't handle functions without an argument well. It expects at least a space after the function name.

In src/function.c, lookup_function, changing

if (e == s || !STOP_SET(*e, MAP_NUL|MAP_SPACE))

to something like

if (e == s || !STOP_SET(*e, MAP_NUL|MAP_SPACE|stopchar_map[(int)')']))

does make it work, if using variable_buffer_output instead of xstrdup in the func_this_* methods.

I do wonder if and under what parameters the original patch works.

func_this_file (char *o UNUSED, char **argv UNUSED, const char *funcname UNUSED)
{
if (reading_file) {
return xstrdup(reading_file->filenm);
Copy link
Collaborator

Choose a reason for hiding this comment

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

using o = variable_buffer_output and return o instead of xstrdup and the internal function(s) do work if adding a pseudo argument, like $(this_file fluffy).

Then it's down to the handling of no-arg functions.

if (!argc && !entry_p->alloc_fn)
if (!argc
/// the functions named this_* by <basile@starynkevitch.net> take no arguments...
&& strncmp(entry_p->name, "this", sizeof("this")-1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my untrained eye sizeof() for a string doesn't seem like a good idea, why not use strlen?

Copy link

@bstarynk bstarynk Jan 12, 2021

Choose a reason for hiding this comment

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

For a constant string, sizeof is computed at compile time.

But strlen might be computed at runtime. I know that some versions of GCC + GNU libc are able to compute strlen at compile time (on Linux/x86-64 at least).

@rocky
Copy link
Owner Author

rocky commented Jan 12, 2021

@boretom Many thanks for investigating what's up, finding and fixing (at least some of) the problems and the review.

Also, all the best for 2021!

@rocky
Copy link
Owner Author

rocky commented Jan 12, 2021

After the dust settles, we should look into adding a few more tests. It shouldn't be that onerous. (In fact, this is something I think even I could do.) Thanks again.

@rocky rocky force-pushed the remake-4-3 branch 5 times, most recently from 269a076 to 3048790 Compare January 22, 2022 15:39
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.

None yet

3 participants