[PATCH] D117921: Attributes: add a new allocalign() attribute

[Augie Fackler via Phabricator via llvm-commits]

durin42 added a comment.

In D117921#3269526 <https://reviews.llvm.org/D117921#3269526>, @reames wrote:

> In D117921#3269497 <https://reviews.llvm.org/D117921#3269497>, @durin42 wrote:
>
>> In D117921#3262394 <https://reviews.llvm.org/D117921#3262394>, @nikic wrote:
>>
>>> If you do want this to be a function int attribute, then the parameter reference should be zero-based, not one-based, for consistency with allocsize.
>>
>> I can't do it zero-based, because there's no way to store a zero in an int attribute. I can go to the work of adding getters and setters everywhere and then pretend it's 0-based for the public API, but internally you'll fail assertions if you try to carry 0 as the payload of an int attr. If you want the getters and setters let me know, I guess? This will matter for one other attribute we need too (`mustfree(N)` - to indicate a pointer will be freed).
>
> Er, this seems strongly suspect to me.  I have not looked carefully, but I'm pretty sure we do use zero based indexing in allocsize.  You should be able to parallel that structure.  (You don't need the argument packing bit, but at the same time it does no harm to reuse if it simplifies the code.)

allocsize is different because it has two parameters, one optional. It's not possible for both to be set to zero, so the payload it carries will _never_ be zero. It does use zero-based indexing, but we'll have to do custom getters/settings across the board like allocsize if we want to use zero-based indexing in both allocalign and mustfree. If that's the preference, I'm happy to do it, but it does seem like a fair amount of extra code for only marginal benefit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117921/new/

https://reviews.llvm.org/D117921