> First of all, I think it's a huge improvement of the RTE_PTR_ADD/SUB macros > to not discard qualifiers anymore. > Silently discarding qualifiers will cause bugs (discarding volatile) and > potentially degrade performance (discarding const).
Yay! It's been an iterative process but I agree it is converging to a better result. > And I agree with your approach of generally fixing instances where this > qualifier-discarding property is abused across existing code. > Qualifiers should only be explicitly discarded. > > Unfortunately, fixing the macros changes their behavior, which may be > considered API breakage. > Personally, I consider the qualifier-discarding property of the current > macros a bug, which makes your patch a bug fix. > I would like the opinion of other community members on this! > It seems a patch can be both a bug fix and an API break. ;-) I agree this is a breaking change for these APIs. Just to clarify that is OK for inclusion in main (for next LTS) and just debating how/if we backport, correct? If so, another option is to deprecate the existing APIs, stop using them in DPDK, and introduce implementations in this patch as a new API (e.g. RTE_PTR_ADD_TYPED) that is used instead. > Second, I think RTE_INT_PTR_ADD/SUB macros are superfluous. > Just use standard + and - operations when operating on integers, including > "integers as pointers". > > I guess existing instances of RTE_PTR_ADD/SUB operating on "integers as > pointers" can be fixed across existing code to either use standard +/- > operators or - probably more correct - use actual pointer types instead of > integer types. Good topic. The current RTE_INT_PTR_ADD macros are doing 2 things: 1. Offsetting by an amount 2. Safely converting to void* to prevent bugs where compilers assume aligned memory, which isn't guaranteed generally, and may crash at runtime (which I did in my first attempt which preserved pointer return type). For (1), agreed for strict integer operation: use native operators (e.g. + or -). I tried to take that approach in this PR for clear-cut-cases. For (2) I think there is still merit in having a macro to convert between domains safley (e.g. int -> pointer). This macro implementation would be the same as RTE_PTR_UNQUAL, but I think it's worth adding an explicit macro bcz the semantics/motivation may be different (and maybe we find a better way to do the conversion in the future). I can add RTE_PTR_FROM_INT(..) or similar to see what people think.
