On Mon, 2022-03-14 at 16:13 -0700, Delyan Kratunov wrote: > Thanks, Andrii! The nits/refactors are straightforward but have some questions > below: > > On Fri, 2022-03-11 at 15:27 -0800, Andrii Nakryiko wrote: > > > + > > > + /* sanitize variable name, e.g., for static vars inside > > > + * a function, it's name is '<function name>.<variable name>', > > > + * which we'll turn into a '<function name>_<variable name>'. > > > + */ > > > + sanitize_identifier(var_ident + 1); > > > > btw, I think we don't need sanitization anymore. We needed it for > > static variables (they would be of the form <func_name>.<var_name> for > > static variables inside the functions), but now it's just unnecessary > > complication > > How would we handle static variables inside functions in libraries then? Ah, just realized 31332ccb7562 (bpftool: Stop emitting static variables in BPF skeleton) stopped that. I'll remove the sanitization then. [...] > > we don't know the name of the final object, why would we allow to set > > any object name at all? > > We don't really care about the final object name but we do need an object name > for the subskeleton. The subskeleton type name, header guard etc all use it. > We can say that it's always taken from the file name, but giving the user the > option to override it feels right, given the parallel with skeletons (and what > would we do if the file name is a pipe from a subshell invocation?). In particular, with the current test setup, if we don't use the name param, we'd end up inferring names like test_subskeleton_lib_linked3 from the filename. The tests case is a bit contrived and we can work around it but there may be other similar situations out there. -- Delyan
