<div dir="ltr">Ah, thanks, you got to it before I did. I'll test everything, check your patches, and re-land!<div><br></div><div>I also (now that I'm using a real keyboard instead of a phone) wanted to more fully respond to your concerns about incomplete types. I think this is an important point, so its worth being thorough in the examination of the options here.</div><div><br></div><div>The reason to use an incomplete type, much as you indicate, is as some form of abstraction boundary: specifically, not relying on *any aspect* of how that type is implemented, merely that it is a type. Currently, the only thing you can rely on is that it isn't an enum or a primitive type. I actually think it would be great if the language even hid *that* detail from a forward declaration. They should (IMO) work very much like "typename T". You know its a type, but that is all.</div><div><br></div><div>The question becomes, is it reasonable to use such a construct in a PointerIntPair (and consequentially in a PointerUnion or a DenseMap pointer key).</div><div><br></div><div>I would argue that the only thing reasonable to do with such a type in a PointerIntPair is to declare the PointerIntPair. The data layout and size of such a PointerIntPair is unambiguous and clear. Similarly, it is reasonable to declare a pointer.</div><div><br></div><div>But I think it is not reasonable to *use* the PointerIntPair any more than it is reasonable to dereference the pointer. Unfortunately, use of the PointerIntPair happens even when you assign, copy, or move it. But this is inherent in the nature of the beast: in order to know how to behave, PointerIntPair must know how the pointee type is *implemented*. So you can have a member, but you will need to define your methods that operate on it out-of-line and provide the full definition. This still allows circular references and other common patterns, but it is more restrictive than a raw pointer. I think that is, again, in the nature of the beast. We are fundamentally relying on more than just the fact that it is a pointer -- we rely on the particular alignment that pointer will always have.</div><div><br></div><div>The example that fully convinced me of all of this goes as follows. When you forward declare a class, all uses of the forward declared class should be reasonable for *any and all possible implementations* of that class. There should never be a dependence on how the class implemented. But with PointerIntPair, if I replace the implementation of a class with, for example, a single unsigned char that is an index into a single table of all 255 possible instances, essentially forming a *very* compressed pimpl, it would actually break anyone using that class in a PointerIntPair. And it would do so silently in many cases! For me, this makes it really clear that PointerIntPair is actually depending on a particular implementation, and can't correctly work with an incomplete type.</div><div><br></div><div>Hope this helps explain my reasoning, and thanks again for the very quick polly fixes, and apologies again for not getting polly fixed first.</div><div>-Chandler</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Dec 30, 2015 at 12:31 PM Tobias Grosser <<a href="mailto:tobias@grosser.es">tobias@grosser.es</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 12/30/2015 07:29 PM, Chandler Carruth wrote:<br>
> The right solution is to not use incomplete types here as all of my<br>
> other commits indicated. Sorry I didn't catch the uses in Polly last<br>
> night. They just need to be fixed like every other LLVM project. I'll<br>
> finish this morning and re-land.<br>
<br>
OK. I committed in r256649 and r256650 fixes for the two occurrences in<br>
Polly. To my understanding, it should now be save to re-land your<br>
commit. Regarding my Polly changes, I would appreciate a post-commit review.<br>
<br>
I left commit r256620 for you to re-apply, but can easily add it back if<br>
you prefer.<br>
<br>
Best,<br>
Tobias<br>
</blockquote></div>