> From: Bruce Richardson [mailto:bruce.richard...@intel.com] > Sent: Thursday, 22 September 2022 13.59 > > On Thu, Sep 22, 2022 at 12:52:42PM +0100, Bruce Richardson wrote: > > On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Brørup wrote: > > > Checking a const pointer for alignment would emit a warning about > the > > > const qualifier being discarded. > > > > > > No need to calculate the aligned pointer; just check the last bits > of the > > > pointer. > > > > > > v2: > > > - Remove compiler attribute ((const)) from function; > > > it was a coding style issue. > > > > > > Signed-off-by: Morten Brørup <m...@smartsharesystems.com> > > > --- > > > lib/eal/include/rte_common.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/eal/include/rte_common.h > b/lib/eal/include/rte_common.h > > > index 2e22c1b955..ed81e0db0a 100644 > > > --- a/lib/eal/include/rte_common.h > > > +++ b/lib/eal/include/rte_common.h > > > @@ -404,9 +404,9 @@ static void > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) > > > * True(1) where the pointer is correctly aligned, false(0) > otherwise > > > */ > > > static inline int > > > -rte_is_aligned(void *ptr, unsigned align) > > > +rte_is_aligned(const void * const __rte_restrict ptr, const > unsigned int align) > > > { > > > - return RTE_PTR_ALIGN(ptr, align) == ptr; > > > + return ((uintptr_t)ptr & (align - 1)) == 0; > > > > Are we confident that in future, or using come compiler settings, we > won't > > get an error due to using "uintptr_t" rather than "const uintptr_t" > in the > > cast? I would put a const in there myself, just to be safe.
Good idea. > > > > A further point, only-semi-related to this patch, which is fine as- > is: > > looking at the code for the various macros in rte_common.h: > > * The various macros for working on pointers can can probably be > converted > > to functions, since they don't need to work with variable-sized > types. > > * We can then see about properly ensuring those inline functions are > > const-correct. The problem with const in a function parameter is the ripple effect: all the underlying functions must also use const. I generally prefer using const where possible, but the ripple effect often makes it difficult. > > > Actually, on further investigation in trying this, it appears that the > macros are used in a number of places with integer data too, despite > the > "PTR" in the name, so things are best alone for now, I think. Even the macros that also exist without "PTR" in the name? (Example, please.) Instead of providing multiple macros for essentially doing the same thing to different types, we could use __builtin_choose_expr [1] to support a variety of types in the macros. This built-in can be used as a workaround for not being able to use C++, where the same function name can be used by multiple functions with different parameter types. [1]: https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html I was also wondering why the rte_is_aligned function doesn't have "ptr" in its name, because it cannot be used for integer types. Changing rte_is_aligned to a macro using __builtin_choose_expr could solve this. But I don't think such a patch will be popular, so I chose to stick with the simple fix. -Morten
