240325_wxrc_FileToCppArray_01h #24437

DoctorNoobingstoneIPresume · 2024-03-25T07:27:00Z

wxrc: FileToCppArray and MakePackageCPP:

We no longer claim to support resources larger than 4 GiB. => [2024-03-27] Commit message has now been changed to "Resource size is now limited to 4 GiB on all platforms." to reduce negativity, oh no, I mean to enhance positivity. (-:
We have replaced new [] and delete [] with std::vector <...>.
Temp file info (index, size, name) is now printed before defining each byte-array.
static has been replaced by anonymous namespace. const has been added.
Byte-array initialization integer literals are now hex; progress is shown.

This pull request replaces part of #24413 (as a sequence of more focused commits).

Thank you for considering this and for kind comments !! 🤗

DoctorNoobingstoneIPresume · 2024-03-25T10:00:15Z

I apologize. I am not sure why 8 commits are included in my pull request. Only 5 of them have been intended to be included... :'( I am going to look into it...

DoctorNoobingstoneIPresume · 2024-03-26T09:02:59Z

The following commit has now been added, beyond the scope of #24413:

wxrc: GetInternalFileName: Call to Printf: Format string arg is now constant.

The following commit has now also been added, completing (in my opinion) the replacement of #24413:

wxrc: MakePackageCPP: Faster&leaner compilation of C++ source code generated by our resource compiler.

…platforms.

…h `std::vector <...>`.

…nted before defining each byte-array. An example of output (generated C++ code): ``` // Temporary file 7h of C9h ( 3.48%) (of size 68h) '/home/ubuntu/Projects/com_github/wxWidgets/wxWidgets-build/Example2.cpp$ExampleImages_Example_0007h.png': static size_t xml_res_size_0 = 104; static unsigned char xml_res_file_0[] = { ...}; ``` The size is printed before the name for nicer alignment of lines (because the size is always 8 hex digits, but the name can have variable character length).

…pace. `const` has been added.

vadz

Thanks for the update!

I think I'd rather move the (useful, thank you) explanations of the changes to the commit messages and leave just the brief summary in the comments in the code, but I can do it myself when merging this.

The biggest remaining question is whether we want the extra complexity of dealing with the sections in the output. IMO this is not really useful and I don't understand why would anybody want to look at these files, they're really meant to be only used as an input to the compiler and never read by a human. So I'd like to exclude the commits changing this when merging.

But I'd like to know what do the others think about it, would anybody else find it useful to have the section headers in the generated output?

vadz · 2024-04-08T18:00:15Z

utils/wxrc/GenerateBigXRCFile.sh

+    for ((i = 0; i < n; ++i)); do
+        local pathname_image; printf -v pathname_image "%s/Example_%04Xh.png" "${folder_images}" "$((i))"
+
+        if ((1)); then


Sorry, what's the purpose of this line?

Thank you. I have now removed it (and the corresponding fi of the if):
https://github.com/wxWidgets/wxWidgets/compare/5004644da73cb23fbe3d26ba172b05cfabb67685..9f5fdc57d3430384c07c20c61d8b37b6be1efac9

vadz · 2024-04-08T18:00:43Z

utils/wxrc/GenerateBigXRCFile.sh

+            printf >>"${pathname_image}" "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A\x00\x00\x00\x0D\x49\x48\x44\x52"
+            printf >>"${pathname_image}" "\x00\x00\x00\x10\x00\x00\x00\x10\x08\x02\x00\x00\x00\x90\x91\x68"
+            printf >>"${pathname_image}" "\x36\x00\x00\x00\x09\x70\x48\x59\x73\x00\x00\x0E\xC4\x00\x00\x0E"
+            printf >>"${pathname_image}" "\xC4\x01\x95\x2B\x0E\x1B\x00\x00\x00\x1A\x49\x44\x41\x54\x28\xCF"
+            printf >>"${pathname_image}" "\x63\x6C\x60\xF8\xCF\x40\x0A\x60\x62\x20\x11\x8C\x6A\x18\xD5\x30"
+            printf >>"${pathname_image}" "\x74\x34\x00\x00\xC5\xBF\x01\x9F\x22\x91\xFF\xBD\x00\x00\x00\x00"
+            printf >>"${pathname_image}" "\x49\x45\x4E\x44\xAE\x42\x60\x82"


Do we really have to compose the file piece-wise like this? Why can't we just add it to the repository?

I was thinking we have a completely independent single-file script, but we can of course move the actual content to an external file if you wish.

DoctorNoobingstoneIPresume · 2024-04-09T14:11:45Z

Thanks for the update!

Thank you for everything !!

I think I'd rather move the (useful, thank you) explanations of the changes to the commit messages and leave just the brief summary in the comments in the code, but I can do it myself when merging this.

Ok, I was thinking that we do not normally have commit messages which are this long, and also to (maybe by force) get the message across to more people 😈 , but either way is fine with me, and I can modify the comment/commit messages and also you please feel free to do it as you think is best.

The biggest remaining question is whether we want the extra complexity of dealing with the sections in the output. IMO this is not really useful and I don't understand why would anybody want to look at these files, they're really meant to be only used as an input to the compiler and never read by a human. So I'd like to exclude the commits changing this when merging.

But I'd like to know what do the others think about it, would anybody else find it useful to have the section headers in the generated output?

My opinion has been in this reply to the previous pull request: #24413 (comment).

For review of the output, here is the XRC output of running './Example_Generator.sh' 100 (i.e. XRC file referencing 100 files):

https://sharetext.me/go6bnohaon

and here are the .h file and .cpp file generated by processing that XRC file with wxrc:

In the latter file (the .cpp file), we have the C++ array representation of 100 short PNG files and a longer file (the XRC file itself). When scrolling through the representation of the longer file, I think the section headers (every 16 lines, i.e. every 256 bytes) might help.

Also, I think that the C++ array representations of the binary files might be useful for debugging. For example, the author might think she has given an UTF-8 encoded file, and might wonder why she does not see the expected glyphs, and in order to debug she might run hexdump -Cv input_file.txt and compare that with the hex representation in the wxrc-generated C++ code (at the exact offsets where non-ASCII code points are expected) => helping her with offsets might be useful.

However, I am willing to remove these section headers if you and/or the community thinks otherwise, or you can also remove them as you see fit if you will.

…e now hex; progress is optionally printed. 16 bytes are output on each line. Previously, a variable number of bytes used to be output on each line, depending on the character length of the decimal representation of each bytes. Indeed, the number of output characters used to be lower; the increase is around 20%; but the representation is nicer for some people (I hope). Progress (offset, total size, percentage) is printed (as C++ comments) if the user so demands (via e.g. `--dump-section-size 256`); in this case, progress is also printed at the beginning and at the end (of the per-file dump). Example of output (generated C++ source code): ``` // Temporary file 64h of 65h ( 99.01%) (of size 2723h) 'C:\Adder\Projects\com_github\wxWidgets\Example2.cpp$._Example2.xrc': const size_t xml_res_size_100 = 10019; const unsigned char xml_res_file_100[] = { // Offset 0h of 2723h ( 0.00%): 0x3c,0x3f,0x78,0x6d, 0x6c,0x20,0x76,0x65, 0x72,0x73,0x69,0x6f, 0x6e,0x3d,0x22,0x31, 0x2e,0x30,0x22,0x20, 0x65,0x6e,0x63,0x6f, 0x64,0x69,0x6e,0x67, 0x3d,0x22,0x55,0x54, 0x46,0x2d,0x38,0x22, 0x3f,0x3e,0x0a,0x3c, 0x72,0x65,0x73,0x6f, 0x75,0x72,0x63,0x65, 0x20,0x78,0x6d,0x6c, 0x6e,0x73,0x3d,0x22, 0x68,0x74,0x74,0x70, 0x3a,0x2f,0x2f,0x77, 0x77,0x77,0x2e,0x77, 0x78,0x77,0x69,0x64, 0x67,0x65,0x74,0x73, 0x2e,0x6f,0x72,0x67, 0x2f,0x77,0x78,0x78, 0x72,0x63,0x22,0x20, 0x76,0x65,0x72,0x73, 0x69,0x6f,0x6e,0x3d, 0x22,0x32,0x2e,0x35, 0x2e,0x33,0x2e,0x30, 0x22,0x3e,0x0a,0x20, 0x20,0x3c,0x6f,0x62, 0x6a,0x65,0x63,0x74, 0x20,0x63,0x6c,0x61, 0x73,0x73,0x3d,0x22, 0x77,0x78,0x42,0x69, 0x74,0x6d,0x61,0x70, 0x22,0x20,0x6e,0x61, 0x6d,0x65,0x3d,0x22, 0x43,0x61,0x74,0x5f, 0x30,0x30,0x30,0x30, 0x68,0x22,0x3e,0x45, 0x78,0x61,0x6d,0x70, 0x6c,0x65,0x32,0x2e, 0x63,0x70,0x70,0x24, 0x45,0x78,0x61,0x6d, 0x70,0x6c,0x65,0x5f, 0x49,0x6d,0x61,0x67, 0x65,0x73,0x5f,0x45, 0x78,0x61,0x6d,0x70, 0x6c,0x65,0x5f,0x30, 0x30,0x30,0x30,0x68, 0x2e,0x70,0x6e,0x67, 0x3c,0x2f,0x6f,0x62, 0x6a,0x65,0x63,0x74, 0x3e,0x0a,0x20,0x20, 0x3c,0x6f,0x62,0x6a, 0x65,0x63,0x74,0x20, 0x63,0x6c,0x61,0x73, 0x73,0x3d,0x22,0x77, 0x78,0x42,0x69,0x74, 0x6d,0x61,0x70,0x22, 0x20,0x6e,0x61,0x6d, 0x65,0x3d,0x22,0x43, 0x61,0x74,0x5f,0x30, 0x30,0x30,0x31,0x68, 0x22,0x3e,0x45,0x78, 0x61,0x6d,0x70,0x6c, // Offset 100h of 2723h ( 2.56%): 0x65,0x32,0x2e,0x63, 0x70,0x70,0x24,0x45, 0x78,0x61,0x6d,0x70, 0x6c,0x65,0x5f,0x49, ... // Offset 2700h of 2723h ( 99.65%): 0x70,0x6c,0x65,0x5f, 0x30,0x30,0x36,0x33, 0x68,0x2e,0x70,0x6e, 0x67,0x3c,0x2f,0x6f, 0x62,0x6a,0x65,0x63, 0x74,0x3e,0x0a,0x3c, 0x2f,0x72,0x65,0x73, 0x6f,0x75,0x72,0x63, 0x65,0x3e,0x0a // Offset 2723h of 2723h ( 100.00%). }; ```

…ow constant. In the original code: ``` s.Printf(wxFileNameFromPath(parOutput) + wxT("$%03i-") + name2, i); ``` if `wxFileNameFromPath(parOutput2)` or `name2` had contained `'%'` (format specifiers), the results would have been bad. These strings are now given as additional arguments to `Printf`, and the format string is now constant.

…generated by our resource compiler. In order for C++-compilation to run faster and consume less memory (in the presence of thousands of input resource files) (especially with g++-14), we have changed `XRC_ADD_FILE` from a function-like macro to a function which takes not `const wxString &` parameters, but instead `const wxChar *` parameters. (Details and benchmarks are in comments in this commit.)

DoctorNoobingstoneIPresume · 2024-04-10T09:34:22Z

The biggest remaining question is whether we want the extra complexity of dealing with the sections in the output. IMO this is not really useful and I don't understand why would anybody want to look at these files, they're really meant to be only used as an input to the compiler and never read by a human. So I'd like to exclude the commits changing this when merging.
But I'd like to know what do the others think about it, would anybody else find it useful to have the section headers in the generated output?

Section headers are now optional (via e.g. --dump-section-size=256) and by default disabled:

https://github.com/wxWidgets/wxWidgets/compare/9f5fdc57d3430384c07c20c61d8b37b6be1efac9..b8907f2525e3ca9c9769a2566b1713b086fe5496

😈

DoctorNoobingstoneIPresume · 2024-04-10T09:36:45Z

I think I'd rather move the (useful, thank you) explanations of the changes to the commit messages and leave just the brief summary in the comments in the code, but I can do it myself when merging this.

Ok, I was thinking that we do not normally have commit messages which are this long, and also to (maybe by force) get the message across to more people 😈 , but either way is fine with me, and I can modify the comment/commit messages and also you please feel free to do it as you think is best.

Perhaps someone wants to later add benchmarks for another toolchain (e.g. clang++) -- then he or she can make a commit which modifies/extends the comments in wxrc.cpp. I think this would be more difficult if the benchmarks were in the commit message...

vadz · 2024-04-25T14:43:30Z

Sorry for the delay with coming back to it and thanks for splitting this into separate commits. I'd like to make some comments for each of them:

48c3f2c (wxrc: FileToCppArray: Resource size is now limited to 4 GiB on all platforms.): I agree with the change, but I think we should keep using size_t and just compare that the length of the file is not greater than 4 (or even maybe 2) GiB. I also still think that the explanatory comment could be much shorter and just say that we want the generated file to compile on all systems, including 32 bit ones. Finally, we definitely should use actual error checking and wxLogError() instead of wxASSERT_MSG() because the failure of the condition being checked doesn't indicate a problem in the program logic but one with its inputs.
a3291f8 (wxrc: FileToCppArray: We have replaced new [] and delete [] with std::vector <...>.): No comments, definitely a good change, thanks.
868c382 (wxrc: FileToCppArray: Temp file info (index, size, name) is now printed before defining each byte-array., 2024-03-25) and 73a99c5 (wxrc: FileToCppArray: Byte-array initialization integer literals are now hex; progress is optionally printed.): Thanks for making this optional but unless someone else votes for having this, I'd still rather omit these commits, they just seem to add more code without any real benefit for 99.99% of wx users.
4f82001 (wxrc: FileToCppArray: static has been replaced by anonymous namespace. const has been added.): const is good but I see no real advantage of replacing static. Is there any reason to do it?
9a5d32d (wxrc: GetInternalFileName: Call to Printf: Format string arg is now constant.): Good change, thanks.
b8907f2 (wxrc: MakePackageCPP: Faster&leaner compilation of C++ source code generated by our resource compiler.): Also good change, but I'd rather put most of the explanations into the commit message than in the comment in the source.

To summarize, I'd like to apply (2) and (5) as is, (1) and (4) with minor changes and (6) with just some comment reformatting/moving, while omitting (3).

But I'll wait for a bit to see if anybody else wants to have (3). And, of course, please let me know if you have any comments.

DoctorNoobingstoneIPresume · 2024-05-20T23:40:20Z

Thank you for kind answer. My partial answer follows:

[...]
1. 48c3f2c (wxrc: FileToCppArray: Resource size is now limited to 4 GiB on all platforms.): I agree with the change, but I think we should keep using size_t and just compare that the length of the file is not greater than 4 (or even maybe 2) GiB. I also still think that the explanatory comment could be much shorter and just say that we want the generated file to compile on all systems, including 32 bit ones. Finally, we definitely should use actual error checking and wxLogError() instead of wxASSERT_MSG() because the failure of the condition being checked doesn't indicate a problem in the program logic but one with its inputs.
[...]

Please kindly see #24546.

[...]
5. 9a5d32d (wxrc: GetInternalFileName: Call to Printf: Format string arg is now constant.): Good change, thanks.
[...]

Please kindly see #24547.

vadz · 2024-05-23T00:03:20Z

Your PRs corresponding to (1) and (5) have been merged and I've also pushed 3c41a68 which does what (2) do and more.

So, basically, there is just (6) left here.

DoctorNoobingstoneIPresume mentioned this pull request Mar 25, 2024

wxrc: Faster&leaner compilation of C++ source file generated by resource compiler. #24413

Closed

DoctorNoobingstoneIPresume force-pushed the 240325_wxrc_FileToCppArray_01h branch from 33c983d to ec7d065 Compare March 25, 2024 22:56

DoctorNoobingstoneIPresume added 4 commits March 27, 2024 10:58

wxrc: FileToCppArray: Resource size is now limited to 4 GiB on all …

48c3f2c

…platforms.

wxrc: FileToCppArray: We have replaced new [] and delete [] wit…

a3291f8

…h `std::vector <...>`.

wxrc: FileToCppArray: static has been replaced by anonymous names…

4f82001

…pace. `const` has been added.

DoctorNoobingstoneIPresume force-pushed the 240325_wxrc_FileToCppArray_01h branch 3 times, most recently from 524b53d to 5004644 Compare March 27, 2024 13:50

vadz reviewed Apr 8, 2024

View reviewed changes

DoctorNoobingstoneIPresume force-pushed the 240325_wxrc_FileToCppArray_01h branch from 5004644 to 9f5fdc5 Compare April 9, 2024 14:20

DoctorNoobingstoneIPresume added 3 commits April 10, 2024 12:31

DoctorNoobingstoneIPresume force-pushed the 240325_wxrc_FileToCppArray_01h branch from 9f5fdc5 to b8907f2 Compare April 10, 2024 09:32

This was referenced May 20, 2024

wxrc: FileToCppArray: Resource size is now limited to 4 GiB on all platforms. #24546

Closed

wxrc: GetInternalFileName: Call to Printf: Format string arg is now constant. #24547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

240325_wxrc_FileToCppArray_01h #24437

240325_wxrc_FileToCppArray_01h #24437

DoctorNoobingstoneIPresume commented Mar 25, 2024 •

edited

DoctorNoobingstoneIPresume commented Mar 25, 2024 •

edited

DoctorNoobingstoneIPresume commented Mar 26, 2024

vadz left a comment

vadz Apr 8, 2024

DoctorNoobingstoneIPresume Apr 9, 2024 •

edited

vadz Apr 8, 2024

DoctorNoobingstoneIPresume Apr 9, 2024

DoctorNoobingstoneIPresume commented Apr 9, 2024

DoctorNoobingstoneIPresume commented Apr 10, 2024 •

edited

DoctorNoobingstoneIPresume commented Apr 10, 2024 •

edited

vadz commented Apr 25, 2024

DoctorNoobingstoneIPresume commented May 20, 2024

vadz commented May 23, 2024

240325_wxrc_FileToCppArray_01h #24437

Are you sure you want to change the base?

240325_wxrc_FileToCppArray_01h #24437

Conversation

DoctorNoobingstoneIPresume commented Mar 25, 2024 • edited

DoctorNoobingstoneIPresume commented Mar 25, 2024 • edited

DoctorNoobingstoneIPresume commented Mar 26, 2024

vadz left a comment

Choose a reason for hiding this comment

vadz Apr 8, 2024

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Apr 9, 2024 • edited

Choose a reason for hiding this comment

vadz Apr 8, 2024

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Apr 9, 2024

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume commented Apr 9, 2024

DoctorNoobingstoneIPresume commented Apr 10, 2024 • edited

DoctorNoobingstoneIPresume commented Apr 10, 2024 • edited

vadz commented Apr 25, 2024

DoctorNoobingstoneIPresume commented May 20, 2024

vadz commented May 23, 2024

DoctorNoobingstoneIPresume commented Mar 25, 2024 •

edited

DoctorNoobingstoneIPresume commented Mar 25, 2024 •

edited

DoctorNoobingstoneIPresume Apr 9, 2024 •

edited

DoctorNoobingstoneIPresume commented Apr 10, 2024 •

edited

DoctorNoobingstoneIPresume commented Apr 10, 2024 •

edited