Re: Cleanup patches

[Igmar Palsenberg <musl@palsenberg.com>]

> On Mon, Jun 06, 2011 at 07:13:18PM +0200, Szabolcs Nagy wrote:
>> %.o: $(ARCH)/%.s
>> -	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
>> +	$(CC) -c -o $@ $<
>> 
>> this could be $(AS) -o $@ $<
> 
> Is there a reason this is necessary or beneficial?

Clang complains about them. You could ignore them if you want : Werror doesn't seem to be affected by this. It might use an internal assembler, I'm not that familiar with clang internals. Looking at the flags, they don't seem to matter when it comes to assembling .s files.

>> struct chunk {
>> -	size_t data[1];
>> 	struct chunk *next;
>> 	struct chunk *prev;
>> +	size_t data[];
>> };
>> 
>> this does not seem to be correct
>> c->data[-1] now means something different
> 
> This is definitely wrong. data[1] is not a flexible array member. If
> this is causing serious problems I could update everything to use
> structure pointers offset back by 1 word, and then use size_t
> prev_size, size; or similar. But if I remember correctly that was
> making the code a good big uglier when I first tried it.

I need to check. I get out of bounds warning with this code. I'll check and update this code.


>> -	    int enter_tag;
>> 	    node = tre_stack_pop(stack);
>> 	    if (first_pass)
>> 		node->num_tags = ((tre_iteration_t *)node->obj)->arg->num_tags
>> 		  + (int)tre_stack_pop(stack);
>> 	    else
>> -		enter_tag = (int)tre_stack_pop(stack);
>> +		(int)tre_stack_pop(stack);
>> 
>> the (int) cast can go as well..
> 
> Indeed. Short of bugs though my intent is not to touch the TRE code,
> since I plan to replace it anyway.
> 
>> 		/* Handle literal text and %% format specifiers */
>> 		for (a=s; *s && *s!='%'; s++);
>> -		litpct = wcsspn(s, L"%")/2; /* Optimize %%%% runs */
>> +		litpct = wcsspn(s, (wchar_t *)L"%")/2; /* Optimize %%%% runs */
>> 		z = s+litpct;
>> 		s += 2*litpct;
>> 		l = z-a;
>> 
>> this seems wrong
>> L"" is already a wchar_t string literal
> 
> I'm guessing this might be an issue of some 32-bit x86 compilers
> disagreeing on whether wchar_t is "int" or "long". Traditionally it
> was "long" which worked but was obviously stupid conceptually. I don't
> know a good way to make musl's wchar.h adapt to what the compiler
> wants though...

The cast should be OK. In cases where it is correct (and the cast isn't necessary), it is simply a NOOP.

>> and wcsspn arguments must be const qualified
> 
> This is wrong. A non-const-qualified pointer always implicitly
> converts to the const-qualified version.

Indeed.

>> Subject: [PATCH 6/6] You can't weak alias a static function or variable
>> 
>> you can, at least gcc/ld allows it, it just does not make much sense
> 
> It does make sense to allow it, but I can see how it might be a little
> more work for the compiler and the compiler might not want to support
> it.
> 
>> but the solution is bad, polluting the public namespace is not ok
> 
> Indeed, the names will all need changing if this is necessary.

There is a clang bug somewhere never the less : I get weird errors with the macro, if I expand them directly in the code, I get a different error. It should give the same error in both cases. I'll recheck this. We might want to make those function hidden if that solves the problem.



	Igmar