> From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Monday, 11 September 2023 17.26 > > 07/09/2023 13:10, Morten Brørup: > > > From: David Marchand [mailto:david.march...@redhat.com] > > > Sent: Thursday, 7 September 2023 10.55 > > > > > > On Thu, Sep 7, 2023 at 10:53 AM David Marchand > > > <david.march...@redhat.com> wrote: > > > > > > > > On Thu, Sep 7, 2023 at 10:50 AM Morten Brørup > <m...@smartsharesystems.com> > > > wrote: > > > > > > This 10 value in the comment is easy to miss if some change > with the > > > > > > prefix is done. > > > > > > Mentionning RTE_THREAD_INTERNAL_NAME_SIZE is enough. > > > > > > > > > > I disagree with David's comment to this. > > > > > > > > > > The function documentation is easier to read if the actual > number is also > > > mentioned. > > > > > > > > > > For the best of both worlds, you can add something like this > nearby: > > > > > > > > > > _Static_assert(sizeof(RTE_THREAD_NAME_PREFIX) == sizeof("dpdk- > "), > > > > > "Length of RTE_THREAD_NAME_PREFIX has changed; " > > > > > "the documentation needs updating."); > > > > > > > > And how will it catch the comment about 10 characters ? > > > > > > I mean you still have to re-read the whole documentation and look > for > > > some reference somewhere about 10 characters. > > > > The trick is to put the _Static_assert close to where the expectation > occurs. That makes it easier to find where changes are necessary. > > > > And the _Static_assert can be added at all the locations where changes > would be necessary. (Generally, we should add a lot more _Static_assert > to the code where it makes assumptions about e.g. the ordering of fields > in a struct, such as the vector optimized code.) > > > > Also, the failure message could be improved to include help about what > to look for. > > > > PS: The reference to RTE_THREAD_INTERNAL_NAME_SIZE should remain in > the documentation, so perhaps look for "RTE_THREAD_INTERNAL_NAME_SIZE". > > I agree with David, it is easier to maintain if not mentioning > the exact value in the doc, > and having a mention to RTE_THREAD_INTERNAL_NAME_SIZE is enough > if it defined as "11". > Then we need only one static assert (or RTE_BUILD_BUG_ON) > below the definition to make sure the number is still valid.
Sorry. Bad choice of words. David is obviously right that it is easy to miss if the prefix changes. My intention was to suggest a mitigation, so we could keep the exact value in the comment, making the documentation easier to read. Anyway, no objections from me.
