Re: [Tinycc-devel] [PATCH] Add support for reading thin archive files.

[Reimar Döffinger]

> On 30 Oct 2023, at 16:03, grischka <grishka@gmx.de> wrote:
> 
> On 30.10.2023 08:41, Reimar Döffinger wrote:
>>> +        if (0 == memcmp(h, "!<thin>\n", 8))
>>> +            return AFF_BINTYPE_AR;
>>> "!<thin>\n" looks like a candidate for a symbolic name not unlike ARMAG ?)
>> 
>> It seemed overkill at first since it was only used in one place,
>> but now that it's used in 2 places, yes.
> 
> Hi,  well, it probably shouldn't be used in two places.  You could
> return AFF_BINTYPE_AR_THIN for example to avoid the redundant extra
> check.

Yeah, but it will add a few new "redundant" checks for AFF_BINTYPE_AR_THIN 
instead.
I admit I had hoped it would be possible to handle it right without
such a distinction, but seems it's not possible.

> Is it that in your code I do notice some general tendency to
> duplications?  Like
>            if (is_thin) size = 0;
>            if (is_thin && tcc_add_ar_thin_file(s1, filename, hdr.ar_name, 
> fnames, fnames_size) != 0) {
>            ...
> Plus 2 locations where it reads the "//" long names.

Yes, there is probably some detail polishing to be done.
I did not want to spend too much time on it when - as illustrated
by the other email - there might be fundamental objections.
Especially since it's not right away obvious how to make it better,
I did consider it of course but it turned out a bit tricky.
I'm happy to look more into it if it seems like a useful feature in principle.

> Also in
> "support for codesigning command"- Seem to recall that it already did exist 
> ?!?

All the tests not using -run crashed for me...
But argh, I did indeed miss the configure option.
I will revert, but any objections to enabling codesign by default?
--- a/configure
+++ b/configure
@@ -191,7 +191,7 @@ Advanced options (experts only):
   --config-bcheck=no       disable bounds checker (-b)
   --config-predefs=no      do not compile tccdefs.h, instead just include
   --config-new_macho=no|yes Force apple object format (autodetect osx <= 10)
-  --config-codesign        Use codesign on apple to sign executables
+  --config-codesign=no     do not use codesign on apple to sign executables
   --dwarf=x                Use dwarf debug info instead of stabs (x=2..5)
   Cross build options (experimental):
@@ -320,6 +320,7 @@ case $targetos in
   Darwin)
     dwarf=4
     confvars="$confvars OSX"
+    default_conf "codesign"
     DLLSUF=".dylib"
     if test -z "$build_cross"; then
       cc=`command -v cc`

> "-MP option" - quite some copy & paste (and a very big comment to my taste).

Big comment seemed better than none. And it's 6 lines copy-pasted.
But it's a fair enough criticism, so I'll attach a refactor attempt.

> "delete created ar file on error" - "created_file": 'fh' already
> means the file was created and argv[i_lib] has the name.

The issue is that fh is ALSO used to open the file for reading.
I kind of missed that that path used a different exit point, but
even then the risk of deleting user data seems a bit worrying.
It could be solved by moving the code to read and create to
different functions, if that sounds reasonable.
Then I would be comfortable with the assumption that fh != NULL
means "delete the file".

> Anyway, as we know, providing two (or more) things to do (almost) the
> same is a well proven method to cause confusion.  Try to get rid of that.

Using one thing for 2 completely different things (reading and deleting for
fh for example) IMO is at least as dangerous.

> Also, not necessarily was tcc written with any not yet existing
> features already in mind.  If you can organize existing stuff differently
> to make the new feature fit more nicely, then why not ... ;)

Also more risk of breaking existing things though.
A bit dangerous to do that in an unfamiliar code-base.

Best regards,
Reimar

0001-tcctools.c-reduce-duplicated-code-for-MP-option.patch
Description: Binary data