[PATCH] D121063: [AST] Make the last element in the linked list null

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

In D121063#3364815 <https://reviews.llvm.org/D121063#3364815>, @erichkeane wrote:

> In D121063#3364810 <https://reviews.llvm.org/D121063#3364810>, @void wrote:
>
>> In D121063#3364780 <https://reviews.llvm.org/D121063#3364780>, @void wrote:
>>
>>> In D121063#3363852 <https://reviews.llvm.org/D121063#3363852>, @erichkeane wrote:
>>>
>>>> I suspect this works because we never really treated this as a LL, just as a pair of iterators.  Two things:
>>>>
>>>> 1- Can you produce some situation where this is valuable to do?
>>>
>>> Yes. In the randstruct feature I'm working on, if this code isn't there it goes into an infinite loop: https://reviews.llvm.org/D120857. It's possible that the way it's constructing and using the list of Decls is somehow wrong, but I wasn't able to identify how.
>>>
>>>> 2- Can you switch this over so that the NextInContextAndBits initializes to nullptr/0 so that this line isn't necessary?  I can't imagine we ever re-call this on a decl and have the answer be different.
>>>
>>> I'll give it a shot. I'm with @urnathan though that it should have already been like that. :-) Probably just an oversight.
>>
>> After looking at the randstruct code again, it's possible that it's not doing the correct thing. (#include "MildShockMeme.h"!) The Decls already have a their pointers set, but then we shuffle them and all Hell breaks loose when we call that function because the end is pointing back to somewhere within the structure beginning. This patch is probably not a bad idea in general, but if you want me to I can fix it on my end.
>
> Ah!  Shuffling declarations in a chain is not likely to do 'good things', I'm surprised that this is the first issue we've seen!.

Same :-)

> I presume that there needs to be a 'RebuildDeclChain' for the purposes of 'RandStruct' that first nulls-out the next-in-context-and-bits.

Without this patch, then yes. It might actually be a better idea to have it in the DeclContext anyway just in case someone else wants to use it.

> Alternatively, perhaps "RandStruct" should be 'randomizing' on the first call to this BuildDeclChain function.

I assume that `BuildDeclChain` is called once (and only once) per Record? Will randomizing when calling `BuildDeclChain` mess up the ABI somehow? Or is it safer to do it here because the ABI is decided afterwards?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121063