On Wed, 2023-10-11 at 17:41 +0100, Alan Maguire wrote:
[...]
> > > + }
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(btf_features); i++) {
> > > + bool *bval = (bool *)(((void *)conf_load) + btf_features[i].conf_load_offset);
> > > + bool match = encode_all;
> > > +
> > > + if (!match) {
> > > + for (j = 0; j < n; j++) {
> > > + if (strcmp(feature_list[j], btf_features[i].name) == 0) {
> > > + match = true;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > + if (match)
> > > + *bval = btf_features[i].skip ? false : true;
> >
> > I'm not sure I understand the logic behind "skip" features.
> > Take `decl_tag` for example:
> > - by default conf_load->skip_encoding_btf_decl_tag is 0;
> > - if `--btf_features=decl_tag` is passed it is still 0 because of the
> > `skip ? false : true` logic.
> >
> > If there is no way to change "skip" features why listing these at all?
> >
> You're right; in the case of a skip feature, I think we need the
> following behaviour
>
> 1. we skip the encoding by default (so the equivalent of
> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag
> to true
>
> 2. if the user however specifies the logical inversion of the skip
> feature in --btf_features (in this case "decl_tag" - or "all")
> skip_encoding_btf_decl_tag is set to false.
>
> So in my code we had 2 above but not 1. If both were in place I think
> we'd have the right set of behaviours. Does that sound right?
You mean when --features=? is specified we default to
conf_load->skip_encoding_btf_decl_tag = true, and set it to false only
if "all" or "decl_tag" is listed in features, right?
> Maybe a better way to express all this would be to rename the "skip"
> field in "struct btf_feature" to "default" - so in the case of a "skip"
> feature, the default is true, but for opt-in features, the default is false.
Yes, I agree, "default" is better as "skip" is a bit confusing.
[...]
