<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jan 17, 2018, at 10:50 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Jan 17, 2018 at 10:44 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="">aprantl@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class="">On Jan 17, 2018, at 10:20 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:</div><br class="m_-2216866235921989083Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, Jan 17, 2018 at 10:12 AM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;">aprantl added a comment.<br class=""><br class="">It looks like this changes the layout of DISubrange from<br class=""><br class="">Flags | Count |<br class=""><br class=""><br class=""><br class="">================<br class="">Comment at: include/llvm/IR/DebugInfoMetadata.h:372<br class="">+<br class="">+ ConstantInt *getCount() const {<br class="">+ if (auto *MD = dyn_cast<ConstantAsMetadata>(getRawCountNode()))<br class="">----------------<br class="">Perhaps:<br class="">`Optional<ConstantInt *> getConstantCount()` ?<br class=""></blockquote><div class=""><br class="">FWIW (& opinions vary, this isn't something that's clearly one way or the other I think) - I generally use an existing empty state if there is one, like nullptr. Adding an extra empty state seems likely to be confusing - if I see Optional<T*> then I assume there's None, null, and the non-null values - and have to think about what the difference might be between None and null.<br class=""></div></div></div></div></blockquote><div class=""><br class=""></div></div></div><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><div class="">That's a good point — do you think Optional<ConstantInt &> would be better, or is this just being silly? :-)</div></div></div></blockquote><div class=""><br class="">I do sort of like it, really - but I'm not sure it works. I forget if we made references work in llvm::Optional, and I don't think std::optional supports references anyway - so probably not a good path to go down.<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><div class="">I just would like to make it very obvious when we have to expect a nullptr from an API, since LLVM tends to also vend lots of guaranteed nonnull pointers (which I feel should really be references), too.</div></div></div></blockquote><div class=""><br class="">Yep - that's the bit that makes it tricky (judgement call, how obvious is it from usage/naming/etc that this might be null, etc), for sure. :/<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>Okay, here is a pragmatic solution then:</div><div><br class=""></div><div> ConstantInt *getConstantCountOrNull()</div><div><br class=""></div><div>-- adrian</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_quote"><div class=""><br class="">- Dave<br class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><div class="">-- adrian</div></div></div><div style="word-wrap: break-word; line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px;" class=""><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204); padding-left: 1ex;"><br class=""><br class="">================<br class="">Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1445<br class=""> Record.push_back(N->isDistinct());<br class="">- Record.push_back(N->getCount());<br class="">+ Record.push_back(-1); // this field has become obsolete<br class="">+ Record.push_back(VE.getMetadataOrNullID(N->getRawCountNode()));<br class="">----------------<br class="">We typically version bitcode by adding a bit to the flags, like this:<br class="">` Record.push_back(N->isDistinct() | 1<<1);`<br class="">This way we don't have to waste the space for the obsoleted field.<br class=""><br class=""><br class=""><a href="https://reviews.llvm.org/D41695" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D41695</a></blockquote></div></div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div><br class=""></body></html>