Cleanup #826

Jan200101 · 2022-01-30T16:15:49Z

Update meson.build

add logic to loop over more lua names (in the future more names might be discovered)
- also fixes error where forcing fallback would compile lua dynamically
disable warnings and errors on dependencies

adding missing includes and checks, correct data types, pointer mess

various functions from string.h were used but never defined
logic was done across multiple different data types with different signedness, got all of them up to snuff
give 0 sized array size of 1 (array of size 0 is illegal, but rewriting the code is out of the scope of this commit)
add preprocessor that marks possibly unused argument as such (does not mean they will get optimized out or anything)
correctly initialize structs with all data needed
work around function pointers and data pointers being used like they are the same (thats illegal)

src/api/system.c

src/rencache.c

adamharrison · 2022-01-30T17:02:54Z

src/rencache.c

@@ -159,7 +160,7 @@ void rencache_invalidate(void) {
 }


-void rencache_begin_frame(lua_State *L) {
+void rencache_begin_frame(UNUSED lua_State *L) {


Ditto above.

removing it moved the UNUSED from this to f_begin_frame

Wouldn't those lua_States be useful in case we add support for multiple windows?

Sure; but we can always add them back in when and if that happens. As it stands, if it's causing a warning, let's just nix it.

It might be nice to keep so that the signature of all rencache functions stays similar.
We cannot remove lua_State from the f_{begin,end}_frame functions so it might be worth to keep this

adamharrison · 2022-01-30T17:03:02Z

src/rencache.c

@@ -200,7 +201,7 @@ static void push_rect(RenRect r, int *count) {
 }


-void rencache_end_frame(lua_State *L) {
+void rencache_end_frame(UNUSED lua_State *L) {


removing it moved the UNUSED from this to f_end_frame

src/rencache.c

src/api/process.c

Jan200101

Some more stuff

This now compiles using zig cc

src/renderer.c

Jan200101

Ran this through CodeChecker and found some additional things

src/renderer.c

src/api/process.c

adamharrison · 2022-02-11T17:04:20Z

src/api/process.c

+      if (new_fds[stream] > STDERR_FD || new_fds[stream] < REDIRECT_PARENT) {
+        for (size_t i = 0; i < env_len; ++i) {
+          free((char*)env_names[i]);
+          free((char*)env_values[i]);


Yeah; this is a good point; ideally though we'd probably have this as separate logic, rather than repeating it. Perhaps goto? That is the standard way of doing exception handling in C.

the problem right now is that the code is not designed around this but I can look into this

Guldoman · 2022-02-11T23:55:06Z

src/renderer.c

          r = (color.r * src.r * color.a + dst.r * (65025 - src.r * color.a) + 32767) / 65025;
          g = (color.g * src.g * color.a + dst.g * (65025 - src.g * color.a) + 32767) / 65025;
          b = (color.b * src.b * color.a + dst.b * (65025 - src.b * color.a) + 32767) / 65025;
-          *destination_pixel++ = dst.a << surface->format->Ashift | r << surface->format->Rshift | g << surface->format->Gshift | b << surface->format->Bshift;
+          *destination_pixel++ = SDL_MapRGBA(surface->format, r, g, b, dst.a);


It looks like the proper way™ makes things a bit slower. On my system this change increases CPU usage by ~7%, both with and without LITE_USE_SDL_RENDERER.

https://github.com/libsdl-org/SDL/blob/a5c610b0a3857d3138f3f3da1f6dc3172c5ea4a8/src/video/SDL_pixels.c#L904

Code should act the same since we have no palette set.

Function calls are more expensive than doing it directly, I guess.

the overhead could come from having to dynamically resolve the symbol.

Another guess would be that the SDL_expand_byte lookup is more expensive than just doing naive bit shifts.

This might be worth investigating further to see where the difference comes from.

adamharrison · 2022-04-03T23:43:48Z

So I'd like to merge this, but I'd like to not lose any performance on the tight loop. Can we just revert that bit, and leave a comment above it that it's out of standard, so that we don't lose the performance, but still keep an eye on it?

- add logic to loop over more lua names (in the future more names might be discovered) - disable warnings and errors on dependencies

- various functions from string.h were used but never defined - logic was done across multiple different data types with different signedness, got all of them up to snuff - give 0 sized array size of 1 (array of size 0 is illegal, but rewriting the code is out of the scope of this commit) - add preprocessor that marks possibly unused argument as such (does not mean they will get optimized out or anything) - correctly initialize structs with all data needed All these were found by generating the project using `meson -Dwarning_level=3 -Dwerror=true`

adamharrison · 2022-04-15T15:34:29Z

OK, I've tested this, and it looks good. Merging.

github-actions bot added Category: Build System Category: C Core labels Jan 30, 2022

Jan200101 commented Jan 30, 2022

View reviewed changes

src/api/system.c Show resolved Hide resolved

adamharrison requested changes Jan 30, 2022

View reviewed changes

adamharrison reviewed Jan 30, 2022

View reviewed changes

src/api/process.c Outdated Show resolved Hide resolved

Jan200101 commented Feb 4, 2022

View reviewed changes

src/renderer.c Show resolved Hide resolved

src/renderer.c Show resolved Hide resolved

Jan200101 commented Feb 6, 2022

View reviewed changes

src/renderer.c Show resolved Hide resolved

src/api/process.c Outdated Show resolved Hide resolved

src/api/process.c Show resolved Hide resolved

adamharrison reviewed Feb 11, 2022

View reviewed changes

Guldoman reviewed Feb 11, 2022

View reviewed changes

Jan200101 added 6 commits April 5, 2022 10:35

Update meson.build

aec3c1c

- add logic to loop over more lua names (in the future more names might be discovered) - disable warnings and errors on dependencies

remove undefined behavior, correct data types

1b136a0

Comment manual bit manipulation to be investigated

d28f350

check for more edge cases, replace multiple cleanups with goto

c7b4da6

remove system specific includes

928ff56

Jan200101 requested a review from adamharrison April 5, 2022 08:39

adamharrison merged commit fff10a2 into lite-xl:master Apr 15, 2022

Jan200101 deleted the cleanup branch April 15, 2022 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup #826

Cleanup #826

Jan200101 commented Jan 30, 2022 •

edited

Loading

adamharrison Jan 30, 2022

Jan200101 Jan 30, 2022

Guldoman Jan 30, 2022

adamharrison Jan 30, 2022

Jan200101 Jan 30, 2022

adamharrison Jan 30, 2022

Jan200101 Jan 30, 2022

Jan200101 left a comment

Jan200101 left a comment

adamharrison Feb 11, 2022

Jan200101 Feb 11, 2022

Guldoman Feb 11, 2022

Jan200101 Feb 20, 2022

Guldoman Feb 20, 2022

Jan200101 Mar 8, 2022

adamharrison commented Apr 3, 2022

adamharrison commented Apr 15, 2022

Cleanup #826

Cleanup #826

Conversation

Jan200101 commented Jan 30, 2022 • 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

Jan200101 left a comment

Choose a reason for hiding this comment

Jan200101 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamharrison commented Apr 3, 2022

adamharrison commented Apr 15, 2022

Jan200101 commented Jan 30, 2022 •

edited

Loading