<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Sep 16, 2014, at 5:28 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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></blockquote><div><br></div><div>In my last patches I had the OptionRegistry really just being a template type that conformed to the get interface. I was having it be a singleton so that we have a way to gradually transition. To make it the context means we'll need to change all the pass constructors and the pass macros up front. My thoughts on how to phase this in are that we first use the template/singleton that I had in my last patches. Then we can update the context to conform to the API, and migrate the macros and pass construction to pass the context in. After that we can remove the default constructors from the passes and replace the template argument with LLVMContext.</div><div><br></div><div>Does that sound like a good roadmap for a transition?</div><br><blockquote type="cite"><div dir="ltr"><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></blockquote><div><br></div>Sounds good to me. How about I clean up my patches based on your recommendations below and start a Phabricator review?</div><div><br></div><div>-Chris</div><div><br><blockquote type="cite"><div dir="ltr"><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></blockquote><blockquote type="cite"><div dir="ltr"><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"><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></blockquote></div><br></div>
</blockquote></div><br></body></html>