<div dir="ltr">Picking this back up, I think that your patch with some cleanups is probably the right starting point. I think the only really substantive issue I currently have is that I'd rather have the OptionRegistry in the LLVMContext rather than in a singleton method. I understand we'll also have to add similar registries to other context objects (MCContext at least), but I think it's useful to start off with just one context initially.<div><br></div><div>How do you want to go about the review / cleanups on the patch? Maybe a new thread? I have a couple of significant things and probably a bunch of really minor cleanup stuff.</div><div><br></div><div>While you may already have much of this fixed, just listing the things I've seen here so I don't forget:</div><div><br></div><div>- I would use a DenseMap or a sorted vector for the lookups. While not super hot, std::map is pretty terrible.</div><div>- Naming of parameters likely needs updating to match conventions...</div><div>- I think you can kill off the hidden flag from the new API and just always mark these as hidden with cl::opt. Any flags which *aren't* marked as hidden probably shouldn't migrate to this API and instead should become constructor parameters controlled at API boundaries (and surfacing with non-hidden cl::opt objects in the tools rather than the libraries).</div><div>- I think you can kill off the builder based on the above. Most of the builder is already redundant with the register API.</div><div>- Go ahead and clang-format the CommandLine.{h,cpp} code you're going to need to touch so that the diff is sane.</div><div><br></div><div>I think everything else I see will just fall out of the cleanup.</div><div><br></div><div>Sound like a plan?</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 29, 2014 at 4:40 PM, Chris Bieneman <span dir="ltr"><<a href="mailto:cbieneman@apple.com" target="_blank">cbieneman@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">This all sounds fine to me.<div><br></div><div>How do you feel about the idea of committing changes with some cleanup from my last patches and converting a few  cl::opts to the new API to see how the new API feels? I can provide patches either later today or this weekend.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Chris</div></font></span><div><div class="h5"><div><br><div><div>On Aug 29, 2014, at 2:14 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <span dir="ltr"><<a href="mailto:cbieneman@apple.com" target="_blank">cbieneman@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>While it is true that we probably don’t need to allow the libraries to mess around with positional parameters, non-hidden options are (I think) a different story. If you look at include/llvm/CodeGen/CommandFlags.h, there are a number of flags defined here that are not positional, not hidden, but only relevant to the CodeGen library. It is probably reasonable that these flags be transitioned to the new API, but they also should be exposed on the command line.</div>
</blockquote><div><br></div><div>So, the 'hidden' flags are *all* exposed on the commandline, they just aren't in the *help* output.</div><div><br></div><div>My thinking (which perhaps is wrong) is that the library options of this form probably shouldn't be in the help output. The help output should probably talk about the tool commandline flags which drive actual API knobs in constructors of things.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div>Aside from those options there are probably very few cases where it actually makes sense for library options to not be hidden, although I must add the caveat that I’m sure there are plenty of library options that are not hidden today and making this change will make them hidden. I’m ok with this, but it does change one of my original goals because it will actually modify the behavior of the command line.</div>
</blockquote><div><br></div><div>This should only change the behavior of '-help' which I think is fine.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Is that a convincing argument for you to restrict the scope of this interface? Maybe there are other things that you'd like to do here that motivate going the other direction?</div>

</div></div></div>
</blockquote><br></div><div>How do you feel about also providing a fluent-style API for toggling the (hopefully) uncommon options?</div></blockquote></div><br>I think that if we have optional things that don't make sense as one or two unsurprising optional arguments, the fluent API design is the right one.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">But I'd like to understand what those optional properties are. Currently, all of the ones we've discussed seem fine to either be required, or be completely omitted.</div>
</div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>