Re: [PATCH 06/25] Generate c for feature instead of tdesc

[Yao Qi <qiyaoltc@gmail.com>]

Pedro Alves <palves@redhat.com> writes:

> On 06/12/2017 09:41 AM, Yao Qi wrote:
>> +$(FEATURE_CFILES): %.c: %.xml.tmp
>> +	$(GDB) -nx -q -batch \
>> +	  -ex 'maint print c-tdesc $<' > $@.tmp
>> +	sh ../../move-if-change $@.tmp $@
>> +	rm $<
>> +
>> +%.xml.tmp: %.xml
>> +	echo "<?xml version=\"1.0\"?>" > $@
>> +	echo "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">" >> $@
>> +	echo "<target>" >> $@
>> +	echo "  <architecture>" >> $@
>> +	if test $(findstring i386/32bit-,$@); then echo "i386" >> $@ ; fi;
>> +	echo "  </architecture>" >> $@
>> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
>> +	echo "</target>" >> $@
>> +
>
> Don't we need move-if-change here?
>

move-if-change from what to what?  *.xml.tmp is removed after *.c is
generated.

> The findstring bits warrants a comment.
>

Yes, comment is needed.

>> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
>> +  Original: 32bit-avx512.xml.tmp */
>
> I don't think we should be pointing "Original" at a 
> temporary file that does not exist in the repo?

Fixed locally.

>
> On 06/12/2017 09:41 AM, Yao Qi wrote:
>> -  print_c_tdesc v (filename_after_features);
>> +  if (strncmp (filename_after_features.c_str(), "i386/32bit-", 11) == 0)
>
> startswith
>
> But again, this looks like needs at least a comment.  I'd
> like to see an expanded rationale for this.  It's not
> immediately obvious why/what's this special casing for.

OK.  Note that the special case and the findstring in Makefile are
needed during the target description transition.  Once we move all
targets to the flexible target description, we don't need them.

-- 
Yao (齐尧)