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? > > > > > - 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 >
