Optimizations by pbfy0 (Rebase, needs testing)

[gameblabla]

Given that pby0 has since passed away, i did a rebase of his branch.
The old PR could be found here :
#123

My branch compiles but i wasn't able to try it on real hardware given that i have since sold my TI Nspire...
Also, i wasn't able to keep the authorship of the commits, given that it required several changes and all and i don't know how to do that... Oh well.

Made a comment here (#123 (comment)) but it would be a waste to not merge back his work so there we go.

[Vogtinator]

This means that every use of any of these variables will also pull in all others via keys.o. Can you compare the impact?

[gameblabla]

Obviously this is a mistake on my part... Let me fix that.

[gameblabla]

Actually i fixed it in gameblabla@7353060 since it wouldn't build ofc but perhaps i should redo my patches and force push ?

[Vogtinator]

You can force push at any time - GH reviews work well with that.

[gameblabla]

Alright so i cleaned up my commits and i also recompiled it just in case to make sure it does compile fine.
Now all it needs is some testing to make sure it's all working fine. (because as i said earlier, i don't have the hardware anymore :/)

[gameblabla]

Compiled a newer build of Picodrive with those changes and gave it to someone else to try. It worked fine, no harmful effects. (although he did not use my newer resources file)

[adriweb]

(oops)

[adriweb]

I may be wrong, but the "Support absolute paths in ndless.cfg." commit seems to me like it would make it possible to execute programs stored in /documents while we're in PTT (i.e. with /documents not being accessible). So, I guess we shouldn't have this patch for obvious cheating-enabling reasons (what's another usecase for that anyway?)

[gameblabla]

I got my TI Nspire CX II today and i can confirm that as of my latest push, it works fine on my calc. I did compile everything with GCC 10.2 instead of GCC 10.1 but that's something you can address later on.

I may be wrong, but the "Support absolute paths in ndless.cfg." commit seems to me like it would make it possible to execute programs stored in /documents while we're in PTT (i.e. with /documents not being accessible). So, I guess we shouldn't have this patch for obvious cheating-enabling reasons (what's another usecase for that anyway?)

Had an issue with PTT mode where i couldn't get out unless i used ndless on it (i didn't have a Windows machine on hand either) so this was useful to me. Not sure though if the commit does fix it (it was not mine) and i don't want to try it either due to the issue i just mentioned : )

[adriweb]

Well there's no issue installing and using Ndless while in PTT. People just have to transfer and launch the installer as usual. That's been that way for a while (a few years/versions) now. It's indeed been useful in the past for un-breaking/un-bricking calcs in weird states. The problem with this specific commit being that it allows for stuff not present in the PTT fake root (but /documents or wherever) to be launched from PTT. I don't think there's any valid usecase for that? This is assuming that Ndless is pre-installed already in PTT though so I guess it's not a nominal case... (I mean, this whole thing is maybe not an actual issue because if Ndless needs to be transferred and installed then people can just do whatever anyway. Still, this commit opens up potentially unwanted things for no valid usecases that I can see :p)

[admsev]

@gooroodev WDYT?

[gooroodev]

@admsev, thanks for the invite!

1. Summary of Changes

The pull request includes the following changes:

1. Refactoring of Key Definitions:

   • Moved the static key definitions from keys.h to a new file keys.c.
   • Changed the key definitions in keys.h to extern declarations.

2. Configuration Management Enhancements:

   • Introduced a new struct cfg_entry to encapsulate key-value pairs.
   • Added functions to manage configuration entries:
     • cfg_get_entry
     • cfg_set_value
     • cfg_put
     • cfg_put_fileext
   • Enhanced the existing configuration functions to support comments and dynamic resizing of the configuration entries array.
   • Added a function cfg_save_file to save the configuration back to a file.

3. File Association Improvements:

   • Modified ld_exec_with_args to use the new configuration functions for file associations.
   • Improved handling of absolute paths and automatic updating of configuration entries.

2. Issues, Bugs, or Typos

Issue 1: Redundant Declaration

• The function const char *cfg_get(const char *key); is declared twice in libndls.h.

Proposed Fix:
Remove the redundant declaration.

diff --git a/ndless-sdk/include/libndls.h b/ndless-sdk/include/libndls.h
index 8bede8c5..9d876ef3 100644
--- a/ndless-sdk/include/libndls.h
+++ b/ndless-sdk/include/libndls.h
@@ -109,7 +109,6 @@ void cfg_set_value(struct cfg_entry *entr, const char *val);
 const char *cfg_get(const char *key);
 struct cfg_entry *cfg_put(const char *key, const char *val);
 void cfg_put_fileext(const char *ext, const char *prgm);
-const char *cfg_get(const char *key);
 void cfg_register_fileext(const char *ext, const char *prgm);
 void cfg_register_fileext_file(const char *fielpath, const char *ext, const char *prgm);
 #define nl_hassyscall(x) _nl_hassyscall(e_##x)

Issue 2: Typo in Function Name

• The function cfg_register_fileext_file has a typo in the parameter name fielpath.

Proposed Fix:
Correct the parameter name to filepath.

diff --git a/ndless-sdk/include/libndls.h b/ndless-sdk/include/libndls.h
index 8bede8c5..9d876ef3 100644
--- a/ndless-sdk/include/libndls.h
+++ b/ndless-sdk/include/libndls.h
@@ -111,7 +111,7 @@ void cfg_put_fileext(const char *ext, const char *prgm);
 const char *cfg_get(const char *key);
 void cfg_register_fileext(const char *ext, const char *prgm);
 void cfg_register_fileext_file(const char *fielpath, const char *ext, const char *prgm);
-void cfg_register_fileext_file(const char *fielpath, const char *ext, const char *prgm);
+void cfg_register_fileext_file(const char *filepath, const char *ext, const char *prgm);
 #define nl_hassyscall(x) _nl_hassyscall(e_##x)
 #define nl_hassyscall(x) _nl_hassyscall(e_##x)

3. General Review of Code Quality and Style

Code Quality:

• Modularity: The refactoring of key definitions into a separate keys.c file improves modularity.
• Memory Management: The use of dynamic memory allocation and reallocation for configuration entries is handled well, ensuring efficient memory usage.
• Error Handling: Adequate error handling is present, particularly in file operations and memory allocations.
• Documentation: The code lacks inline comments explaining the purpose and functionality of complex sections, which would aid in readability and maintenance.

Code Style:

• Consistency: The code follows a consistent style in terms of indentation, naming conventions, and braces placement.
• Clarity: Variable and function names are descriptive, making the code relatively easy to understand.
• Redundancy: There is some redundancy in function declarations and parameter names, as mentioned in the issues section.

Conclusion

Overall, the pull request introduces significant improvements and optimizations to the codebase. Addressing the identified issues and adding inline comments would further enhance the code quality and maintainability.

Yours, Gooroo.dev. I'd love to hear from you! React or reply.