<div dir="ltr">*nod* I'd be OK with that. (personally I'd figure there are few enough callers of this API that it probably won't be misused, but don't mind adding the extra clarity in the name, for sure)</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 17, 2018 at 10:54 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><br><blockquote type="cite"><div>On Jan 17, 2018, at 10:50 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-8471438755805206204Apple-interchange-newline"><div><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"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 17, 2018 at 10:44 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></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"><div><br><blockquote type="cite"><div>On Jan 17, 2018, at 10:20 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-8471438755805206204m_-2216866235921989083Apple-interchange-newline"><div><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"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 17, 2018 at 10:12 AM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></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><br>It looks like this changes the layout of DISubrange from<br><br>Flags | Count |<br><br><br><br>================<br>Comment at: include/llvm/IR/DebugInfoMetadata.h:372<br>+<br>+  ConstantInt *getCount() const {<br>+    if (auto *MD = dyn_cast<ConstantAsMetadata>(getRawCountNode()))<br>----------------<br>Perhaps:<br>`Optional<ConstantInt *> getConstantCount()` ?<br></blockquote><div><br>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></div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><div>That's a good point — do you think Optional<ConstantInt &> would be better, or is this just being silly? :-)</div></div></div></blockquote><div><br>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> </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"><div><div>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><br>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></div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><div>Okay, here is a pragmatic solution then:</div><div><br></div><div>  ConstantInt *getConstantCountOrNull()</div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><div><br></div><div>-- adrian</div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><br><blockquote type="cite"><div><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"><div class="gmail_quote"><div><br>- Dave<br> </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"><div><div><br></div><div>-- adrian</div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><br><blockquote type="cite"><div><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"><div class="gmail_quote"><div> </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><br>================<br>Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1445<br>   Record.push_back(N->isDistinct());<br>-  Record.push_back(N->getCount());<br>+  Record.push_back(-1); // this field has become obsolete<br>+  Record.push_back(VE.getMetadataOrNullID(N->getRawCountNode()));<br>----------------<br>We typically version bitcode by adding a bit to the flags, like this:<br>` Record.push_back(N->isDistinct() | 1<<1);`<br>This way we don't have to waste the space for the obsoleted field.<br><br><br><a href="https://reviews.llvm.org/D41695" rel="noreferrer" target="_blank">https://reviews.llvm.org/D41695</a></blockquote></div></div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></blockquote></div>