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

[Yao Qi <qiyaoltc at gmail dot com>]

Simon Marchi <simon.marchi@polymtl.ca> writes:

>> +FEATURE_CFILES = $(patsubst %.xml,%.c,$(FEATURE_XMLFILES))
>> +
>> +cfiles: $(CFILES) $(FEATURE_CFILES)
>
> If we are going to have different kinds of CFILES, it would be nice to
> rename CFILES (to TDESC_CFILES I suppose) to clarify the purpose of
> each variable.
>

OK, I'll rename CFILES.

>> +
>> +$(CFILES): %.c: %.xml
>>  	$(GDB) -nx -q -batch \
>>  	  -ex "set tdesc filename $<" -ex 'maint print c-tdesc' > $@.tmp
>>  	sh ../../move-if-change $@.tmp $@
>>
>> +$(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;
>
> Should this be
>
>   test -n "$(findstring ...)"
>
> Quotes would be important here, if findstring returns en empty string,
>

I update the code here, and find the "test findstring" is no longer needed.

>> +	echo "  </architecture>" >> $@
>> +	echo "  <xi:include href=\"$(notdir $<)\"/>" >> $@
>> +	echo "</target>" >> $@
>> +
>
> Instead of creating this temporary file, wouldn't it be simpler to
> make "maint print c-tdesc" (or a new "maint print c-feature") take
> directly a path to a feature XML, and generate the C from that?

Actually, I thought about adding a new command, but I find adding temp
file is simpler than modifying GDB to add a new command and accept a
feature XML.  It is just some lines of code in Makefile, with the "test"
"findstring" stuff removed, they are quite simple.

>> @@ -2033,38 +2040,123 @@ public:
>>      printf_unfiltered ("%d, \"%s\");\n", reg->bitsize, reg->type);
>>    }
>>
>> +protected:
>> +  std::string m_filename_after_features;
>> +
>>  private:
>>    char *m_function;
>> -  std::string m_filename_after_features;
>>    bool m_printed_field_type = false;
>>    bool m_printed_type = false;
>>  };
>>
>> +class print_c_feature : public print_c_tdesc
>
> I am really not convinced that such an inheritance is appropriate
> here.  To make print_c_feature inherit from print_c_tdesc, we should
> be able to say that a print_c_feature object _is_ a print_c_tdesc with
> further specialization.  I don't think it's the case here.  If they

No, there is no is-a relationship between print_c_feature and
print_c_tdesc, so it is inheritance, not subtying.

https://en.wikipedia.org/wiki/Inheritance_(object-oriented_programming)

"Inheritance should not be confused with subtyping....in general,
subtyping establishes an is-a relationship, whereas inheritance only
reuses implementation and establishes a syntactic relationship..."

and

"In object-oriented programming, inheritance is when an object or class
is based on another object (prototypal inheritance) or class
(class-based inheritance), using the same implementation (...) or
specifying a new implementation to maintain the same behavior (...)",

> can share some code, I think it would be appropriate for them to have
> a common base class though.

-- 
Yao (齐尧)