Add optimizations, colored output, better help, and more flags. #17
Conversation
| @@ -7,16 +7,78 @@ | |||
| #include "pak_get_file_type.h" | |||
| #include "pak_header.h" | |||
| #include "pak_pack.h" | |||
| #include <stdio.h> | |||
There was a problem hiding this comment.
stdio.h is already included in pak_file_io.h by pak_pack.h.
| @@ -7,16 +7,78 @@ | |||
| #include "pak_get_file_type.h" | |||
| #include "pak_header.h" | |||
| #include "pak_pack.h" | |||
| #include <stdio.h> | |||
|
|
|||
| void red () { | |||
There was a problem hiding this comment.
Short functions could be marked as inline to give compiler a better hint to inline it.
| #include <stdio.h> | ||
|
|
||
| void red () { | ||
| printf("\033[1;31m"); |
There was a problem hiding this comment.
Consider 3 more things here:
- For printing s static string,
puts(char*)with a new line and `fputs(char*, stdout) without a new line is preferred [1] - These strings (this and similar strings below) is used many times, defining it as macros makes literials more clear to read and mainten.
- Colored output are not always enabled for all kinds of terminals, for example, a
docker runwithout the-toption, default behavior should detect support whenever possible, and there should be options to force enable or disable them. Also, a compile-time option could be also used in case packagers want to disable this feature without patching the code.
Extra links for 1:
https://stackoverflow.com/questions/5690979/when-should-i-use-fputs-instead-of-fprintf
https://stackoverflow.com/questions/17238784/c-puts-without-newline
https://stackoverflow.com/questions/2454474/what-is-the-difference-between-printf-and-puts-in-c
Images for 2, using the ci compiled pak_mingw64.exe:
|
|
||
| void printVersion() { | ||
| // get self path | ||
| char selfName[PATH_MAX]; |
There was a problem hiding this comment.
These code seems not used below
| if (ptr != NULL) | ||
| strcpy(selfName, ptr + 1); | ||
|
|
||
| printf(PAK_VERSION_STRING); |
There was a problem hiding this comment.
- puts and fputs is perferred.
- The standalone switch for version should print sonething readable by machine, human-readable stuff can go with the
helptext. - Versions should be defined as macro so it can be easily updated when needed.
|
|
||
| void printChromium() { | ||
| // get self path | ||
| char selfName[PATH_MAX]; |
There was a problem hiding this comment.
Like mentioned above, the path of the exe seems not needed here.
| if (ptr != NULL) | ||
| strcpy(selfName, ptr + 1); | ||
|
|
||
| printf(CHROMIUM_ASCII); |
There was a problem hiding this comment.
This seems to be an easter egg, but this takes more than 1kB of a ~16kB program, could this be optional by a compile-time flag?
| @@ -121,12 +232,15 @@ int pakPackIndexFile(char *indexPath, char *outputFilePath) { | |||
| free(outputFilePath2); | |||
| if (freeFilesPath && filesPath != NULL) | |||
| free(filesPath); | |||
| printf("\033[1;32m\nPacked %s\033[0m\n\n", outputFilePath); | |||
| printf("\npak size: %u\n", pakFile.size); | ||
| printf("version = %u\nencoding = %u\n", myHeader.version, myHeader.encoding); | ||
| printf("\n.pak size: %u", pakFile.size); | ||
| printf(" bytes\n"); |
There was a problem hiding this comment.
This printf could be merged into the printf above.
| @@ -1,5 +1,6 @@ | |||
| cmake_minimum_required(VERSION 2.8) | |||
| project(chrome-pak C) | |||
| cmake_minimum_required(VERSION 3.0) | |||
There was a problem hiding this comment.
Would upgrading minimum version of cmake a good idea? Are there any features used in this pr require to do this? Maybe the cmake version or the existance of the policy could be checked before applying a policy.
Link: https://cmake.org/cmake/help/v2.8.12/cmake.html#command:if
|
@myfreeer Im really f***king dumb at C. It took me two hours to cobble this together. Would you be willing to make the necessary changes? Then you could send a pull request back to me or something |
|
Here is an example of using macros for colored output: https://github.com/recp/cglm/blob/92a8e3816275ecfbebfbaec5af6e4fce4e6f9b84/test/include/common.h#L51 |
From > https://github.com/Alex313031/thorium/tree/main/pak_src
Also includes a build script for linux.