On Fri, Jun 26, 2020 at 6:07 PM Jerin Jacob <jerinjac...@gmail.com> wrote: > > On Fri, Jun 26, 2020 at 5:46 PM David Marchand > <david.march...@redhat.com> wrote: > > > > On Fri, Jun 26, 2020 at 2:06 PM Jerin Jacob <jerinjac...@gmail.com> wrote: > > > > > > On Fri, Jun 26, 2020 at 5:13 PM David Marchand > > > <david.march...@redhat.com> wrote: > > > > > > > > On Fri, Jun 26, 2020 at 1:16 PM Jerin Jacob <jerinjac...@gmail.com> > > > > wrote: > > > > > > > Alternative is to keep variable declaration outside, > > > > > > > as David suggested, and I tend to agree that it is a > > > > > > > bit better. Macro name says 'register'. It is not > > > > > > > 'declare and register'. Also it avoids static-vs-extern > > > > > > > problem completely. The solution allows to keep the > > > > > > > variable declaration untouched and put constructor (macro) > > > > > > > at the end of fine where constructors typically reside. > > > > > > > > > > > > My only concern with that approach is that, We can not save a lot of > > > > > > code duplication > > > > > > with that scheme. ie. it is [1] vs [2]. We can change the MACRO name > > > > > > accordingly if that is a concern. Any suggestions? > > > > > > > > > > > > Let me know your preference on [1] vs [2], I will stick with for the > > > > > > next version. > > > > > > > > > > If there are no other comments, I change RTE_LOG_REGISTER to static > > > > > version > > > > > and RTE_LOG_REGISTER_EXTERN for a non-static version and send the > > > > > next version. > > > > > > > > - Having a macro that does more than what its name tells is > > > > inconvenient. > > > > > > I agree. What could be that name if we want to declare and register? > > > RTE_LOG_DECLARE_AND_REGISTER_EXTERN? > > > > (sorry, gmail ctrl+enter ...) > > > > Or no declaration in macro and leave code as it is. > > Just to understand, Is it some general coding standard? or Just a preference. > I dont see something like that in the Linux kernel. > In this patch, http://patches.dpdk.org/patch/69681/, You are proposing > the declaration in macro. > See: RTE_TRACE_POINT_REGISTER > > I dont see anything wrong with that. Do you have technical rationale > for the above rule?
David, Waiting for your feedback on this to send the next version. If you have a strong preference for keeping declaration out macro then I will respin the v2 based on that. Let me know. > > > > > > > > > > - Having components set log levels at init time in the macro is a bug > > > > to me. > > > > This has been worked around with > > > > rte_log_register/rte_log_register_and_pick_level but the initial > > > > problem is that rte_log_set_level* should only be called by the user. > > > > > > I agree with the below stuff, That's is not introduced by this patch. > > > It is already there. > > > Be it macro or no macro code. > > > > > > I think this patch helps to change to new scheme as it takes care of > > > changing the > > > registration part to commonplace so that we can set to the same level > > > in the future if > > > everyone agrees to it > > > > We will still expose this macro meaning that we will have an API breakage > > later. > > So if we go with introducing this, let's make it good from the start. > > But Is everyone OK with removing the "level" from the register before > we think about > breaking the API later?. I see PMD uses multiple levels in the same > PMD for different > purposes. > > > > > > > > -- > > David Marchand > >
