> On 30 Oct 2023, at 16:03, grischka <gris...@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
_______________________________________________ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel