[LLVMdev] [RFC] Removing static initializers for command line options

[Chandler Carruth]

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.

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.

While you may already have much of this fixed, just listing the things I've
seen here so I don't forget:

- I would use a DenseMap or a sorted vector for the lookups. While not
super hot, std::map is pretty terrible.
- Naming of parameters likely needs updating to match conventions...
- 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).
- I think you can kill off the builder based on the above. Most of the
builder is already redundant with the register API.
- Go ahead and clang-format the CommandLine.{h,cpp} code you're going to
need to touch so that the diff is sane.

I think everything else I see will just fall out of the cleanup.

Sound like a plan?



On Fri, Aug 29, 2014 at 4:40 PM, Chris Bieneman <cbieneman at apple.com> wrote:

> This all sounds fine to me.
>
> 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.
>
> -Chris
>
> On Aug 29, 2014, at 2:14 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
>
> On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <cbieneman at apple.com>
> wrote:
>
>> 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.
>>
>
> So, the 'hidden' flags are *all* exposed on the commandline, they just
> aren't in the *help* output.
>
> 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.
>
>
>>
>> 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.
>>
>
> This should only change the behavior of '-help' which I think is fine.
>
>> 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?
>>
>>
>> How do you feel about also providing a fluent-style API for toggling the
>> (hopefully) uncommon options?
>>
>
> 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.
>
> 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.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140916/c2390703/attachment.html>