Add another example #24

singul4ri7y · 2024-08-17T13:40:24Z

Changelog:

Added another example (connected-component using DFS)
Separate Makefiles for examples
Example spec tests

…st examples

hugomg

The main issue I see is that the makefiles are too complicated. The examples should be as simple as possible. They should prioritize the needs of the user (which will copy them into their projects) instead of the needs of our test suite.

hugomg · 2024-08-17T18:45:41Z

examples/connected-components/component.c

+    PALLENE_TRACER_GENERIC_C_SETLINE(fnstack)
+
+#define CON_C_FRAMEEXIT()                               \
+    PALLENE_TRACER_FRAMEEXIT(fnstack)


These would look less scary if the defines fit in the same line, without \

The PALLENE_TRACER_GENERIC names are way too long

Even nicer would be if the user didn't have to define macros and could use ptracer macros directly.

Sure.

LOUD names are less confusing in my opinion. @srijan-paul also feel the same way I presume.

Users can, but it would be sorta hassle. We have talked about this earlier, it's okay if users define their own macros, it's not huge boilerplate. Besides, the usual way to go would be put this separately in a header file in module code base, so most C translations don't even have to look at these mess (see the multimod test). Writing some simple boilerplate shouldn't hurt, as you are writing frameenter and frameexit repeatedly in every function.

My main issue with the names are

The long names blend together. In PALLENE_TRACER_GENERIC_C_SETLINE, the common PALLENE_TRACER_GENERIC prefix takes almost all the space and the relevant SETLINE is a small part of the end.

We expect to call SETLINE very often, and such a long name makes it inconvenient to type by hand (without defining a helper macro)

One of the original ideas with the GENERIC macros was to have good defaults. We did that for LINE numbers, but not for the fnstack. If we enforced a default for how to use the fnstack, or if we allowed the user to specify a "FNSTACK" macro, then there would be no need for CON_C_SETLINE or CON_C_FRAMEEXIT.

They are not actually "issues" per-se, at-least to my eyes.

Alternative way to do this would be to reduce PALLENE_TRACER_GENERIC to something shorter, inevitably changing the PALLENE_TRACER part, which is prefix for almost everything in Pallene Tracer. Doing something like PALLENE_TRACER_GCS would be silly.

You are supposed to write some macros. Because no matter how hard we abstract, users have to define their own macros one way or the another for Pallene Tracer APIs because a lot of things are in users hands. Users hands won't fall off if they write some extra macros, not to mention they would be copy-pasting most of the part anyway.

Way too much generalized. What if users want to name it "stack" instead of "fnstack"? What if they want to make fnstack global? Or want to pass it as an upvalue? If we do account for all the cases, it would make macro matters even complicated. We did the generic part for setlines because it is only and only __LINE__ + 1 for every users, which can be and should be generalized.

We could use a shorter name like PTRACER_ or even just PT_

It's unfortunate that even the simplest examples need boilerplate macros

For fnstack, it might suffice to ask the user for a single macro that says the name of the fnstack variable. I'm also not against recommending a default name. The idea is to have good defaults for the common cases.

hugomg · 2024-08-17T18:46:49Z

examples/connected-components/Makefile

+# Please refer to the LICENSE and AUTHORS files for details
+# SPDX-License-Identifier: MIT
+
+OUTPUT_FILE = component.so


Can we have a single makefile for all the examples, instead of copy-pasted makefiles for everyone? These makefiles are too complicated to be copy-pasted.

If we did/could, what was the point of detaching them from the root Makefile? I don't find them complicated, atleast compared to the root Makefile that we have. A moderate Makefile is far better than having no Makefile at all.

The main point of detaching them from the root is because we want to compile the examples after we do make install for the library, because that way it tests that the compilation will also work for the users.

I'd be ok copy-pasting trivial makefiles but these makefiles are not trivial. For starters:

Long list of cflags

Non-portable ifneq, MAKECMDGOALS

Multiple phony targets

Pattern rules (despite building a single file)

That was not the main point actually, you can compile examples after installation from root Makefile as well, it's not a big deal. The main point was, as Pallene Tracer have 2 modes (debug and release controlled by PT_DEBUG macro), we have to show users the techniques to use these modes in a Makefile.

I can remember you telling me Makefiles need some love? :3

Using makefile variables is more natural than if statements

I think we should keep the example makefiles as small as possible. For example, we can probably get rid of CFLAGS, the pattern rules, and much more.

hugomg · 2024-08-17T18:49:01Z

examples/connected-components/Makefile

+
+ifneq ($(findstring debug, $(MAKECMDGOALS)), )
+	CFLAGS += -DPT_DEBUG
+endif


if statements in makefiles are complicated and non-portable. Consider

Just turn on DPT_DEBUG all the time

Configure the DPT_DEBUG via variables

Another alternative is to recursivelly invoke make, just like Lua's makefile does for make linux. But IMO that might be too complicated for what we are doing here.

Just turn on DPT_DEBUG all the time

The main point of detaching the Makefiles was to let the users try the examples with and w/o debugging mode enabled. If we turn on PT_DEBUG all the time, it would be better to move the examples build to root Makefile, which just doesn't seem right.

Variables sound good though. Or maybe we can do some redundancy here (I just found out BSD and POSIX make do not support if statements).

hugomg · 2024-08-17T18:49:13Z

examples/connected-components/Makefile

+
+OUTPUT_FILE = component.so
+
+CC         = cc


You don't have to specify the C compiler variable

hugomg · 2024-08-17T18:50:58Z

examples/connected-components/main.lua

+
+local nodes = 12
+local graph = {
+    { 2 }, -- 1st node


Consider [1] = {2} to make the graph easier to read.

hugomg · 2024-08-17T18:55:18Z

spec/examples_spec.lua

+            make debug --quiet
+            ../../pt-lua main.lua ]], cdir))
+    assert(ok, err)
+    assert.are.same(expected_content, output_content)


Why are we compiling and asserting twice?

The first one is w/o Tracebacks, the second one is with tracebacks. Testing twice to ensure we are not doing anything terribly wrong in Pallene Tracer. But there is a nit, we need to run the first assertion with normal system Lua.

Could those be two separate it blocks?

hugomg · 2024-08-17T18:56:24Z

examples/fibonacci/Makefile

+%.so: %.c
+	$(CC) $(CFLAGS) $(LDFLAGS) $(LIBFLAGS) $< -o $@
+
+%.c: ptracer.h


Where is this ptracer.h coming from? There should be no ptracer.h in the current directory.

hugomg · 2024-08-20T19:50:57Z

examples/connected-components/component.c

+    PALLENE_TRACER_GENERIC_C_SETLINE(fnstack)
+
+#define CON_C_FRAMEEXIT()                               \
+    PALLENE_TRACER_FRAMEEXIT(fnstack)


We could use a shorter name like PTRACER_ or even just PT_

It's unfortunate that even the simplest examples need boilerplate macros

For fnstack, it might suffice to ask the user for a single macro that says the name of the fnstack variable. I'm also not against recommending a default name. The idea is to have good defaults for the common cases.

hugomg · 2024-08-20T19:54:53Z

examples/connected-components/Makefile

+# Please refer to the LICENSE and AUTHORS files for details
+# SPDX-License-Identifier: MIT
+
+OUTPUT_FILE = component.so


Using makefile variables is more natural than if statements

I think we should keep the example makefiles as small as possible. For example, we can probably get rid of CFLAGS, the pattern rules, and much more.

Added another example, include Makefiles with examples and spec to te…

58da3ab

…st examples

singul4ri7y requested a review from hugomg August 17, 2024 13:40

Fix bracket string bug in example traceback

a0e8278

singul4ri7y force-pushed the another-example branch from a31402f to a0e8278 Compare August 17, 2024 13:48

Minor nit fix

b0b6e4e

hugomg requested changes Aug 17, 2024

View reviewed changes

hugomg requested changes Aug 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add another example #24

Add another example #24

singul4ri7y commented Aug 17, 2024

hugomg left a comment

hugomg Aug 17, 2024

singul4ri7y Aug 17, 2024

hugomg Aug 18, 2024

singul4ri7y Aug 18, 2024 •

edited

Loading

hugomg Aug 20, 2024

hugomg Aug 17, 2024

singul4ri7y Aug 17, 2024

hugomg Aug 18, 2024

singul4ri7y Aug 18, 2024

hugomg Aug 20, 2024

hugomg Aug 17, 2024

singul4ri7y Aug 17, 2024 •

edited

Loading

hugomg Aug 17, 2024

hugomg Aug 17, 2024

hugomg Aug 17, 2024

singul4ri7y Aug 17, 2024

hugomg Aug 18, 2024

hugomg Aug 17, 2024

hugomg Aug 20, 2024

hugomg Aug 20, 2024


		OUTPUT_FILE = component.so

		CC = cc

Add another example #24

Are you sure you want to change the base?

Add another example #24

Conversation

singul4ri7y commented Aug 17, 2024

hugomg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singul4ri7y Aug 18, 2024 •

edited

Loading

singul4ri7y Aug 17, 2024 •

edited

Loading