On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote: > Radim Krčmář <rkrcmar@xxxxxxxxxx> writes: > > > 2016-04-19 14:58-0400, Bandan Das: > >> Radim Krčmář <rkrcmar@xxxxxxxxxx> writes: > >>> 2016-04-19 13:08-0400, Bandan Das: > >>>> Andrew Jones <drjones@xxxxxxxxxx> writes: > >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote: > >>>>>> > >>>>>> The cscope.files that we generate doesn't include all > >>>>>> source files that are potentially interesting. We should > >>>>>> include all $(ARCH)es and not the just the one configure > >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the > >>>>>> correct path for x86 sources. Generate cscope.files by searching > >>>>>> for all files starting from root. > >>>>> > >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when > >>>> > >>>> I don't think that's a good idea. Just because you don't like to > >>>> see x86 bits when jumping around in arm code doesn't mean you should > >>>> hide all references to a given function. And if the arm code is > >>>> segregated enough that there are no common functions, you won't > >>>> see them anyway. > >>>> [...] But please let's > >>>> not make it difficult to write arm/powerpc tests just because someone's on > >>>> x86 :) How does it make it more difficult to write arm/powerpc tests on x86? I do it all the time. Of course you need to run ./configure --arch=<ARCH> first, but you need to do that to compile anyway. > >>> > >>> In the kernel, environment can override configuration, so you'd do > >>> > >>> % ARCH=arm make cscope > >>> > >>> and it doesn't matter what arch is being compiled. I think this would > >>> be a better solution for kvm-unit-tests too. This is what kvm-unit-tests does, except it takes ARCH from config.mak, instead of making you specify it, and there is no 'everything' option, default or otherwise right now. > >>> > >>> I agree with Drew that other arches would mostly clutter searches if > >>> they were included by default, but we can also add a rule in case > >> > >> Since you mentioned the kernel, it looks like both the kernel/qemu > >> include all by default which makes sense to me. > > > > qemu does, kernel doesn't (e.g. search for sys_sigreturn). > > Not exactly, arch specific(arch/*) files are excluded but there are > others such as sound/* include/* that are not all x86 specific. The > arch segregation is helpful since there would be functions with similar > names (and similar functionality). That's not the case for every file > in x86/ and arm/ of kvm-unit-tests. kvm-unit-tests' cscope find collects all common files and all arch specific files, just as you've described about the kernel. Please give an example of where you couldn't find something you needed with this find, so that I know what the problem is. Also, if there is a problem, we can likely extend the find just a bit, rather than changing it to a wholesale search. > > > > >> As I mentioned above, > >> you definitely don't want to modify a common library function only to > >> realize that you forgot to modify all sites. Looking at all sites adds > >> visibility, not introduce clutter. Steps to modifying a common function's call sites 1) git grep <function> 2) compile test [at least] all architectures > >> > >>> indexing all arches is desired (like when changing the prototype of some > >>> generic function?). > >> > >> kvm-unit-tests is small enough that this can be the default. > > > > I don't see a reason to add results that we'd then have to mentally > > filter regardless of their amount. > > > >> That said, this > >> is a trivial change and I am happy to live with it in my local branch. (Unless > >> ofcourse both Linux and Qemu decide to make this default behavior too!) > > > > I think we'll agree on something that can be merged -- the present > > situation is bad. > > It's not that bad, actually. We are simply arguing over personal > preferences here. Changing my workflow a bit is not hard. :) > > > For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting > > only arch-specific cscope is harder, so having an option to generate > > cscope just for target arch would be nice. Yes, generating cscope just for the target arch is nice, and that's kvm-unit-tests default, as it makes the most sense for the common unit test developer, i.e. somebody not extending the framework, but rather just writing an arch-specific unit test. > > Yes, the option is definitely nice but making it the default, not so much. As should be clear by now, IMHO, the default is not only nice, but correct. Feel free to add a cscope-all target though. > > > (I don't care about the default much, `ARCH=$sth make cscope` is easy > > to type if the default included all.) I can live with that too, but I do care about defaults, and defaults should target the common cases. I haven't yet seen enough justification as to why seeing everything (whether you want to or not) is the best common case for kvm-unit-tests' cscope generation. Thanks, drew -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
