[PATCH] D54726: [libcxx] Support generating linker script for static library
Louis Dionne via Phabricator
reviews at reviews.llvm.org
Fri Dec 21 14:18:15 PST 2018
ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.
Ship it, with named parameters. Don't wait over the holidays.
Comment at: libcxx/utils/gen_link_script.py:22
+ action="store_true", default=False)
+ parser.add_argument("input", help="Path to libc++ library")
+ parser.add_argument("output", help="Path to libc++ linker script")
> ldionne wrote:
> > phosek wrote:
> > > ldionne wrote:
> > > > Please consider making these named parameters instead of positional parameters. That would have answered some of my questions before I had to ask them.
> > > I find it little awkward to use optional arguments as required arguments but I can do that if you prefer?
> > You can have required named arguments: `parser.add_argument('--foo', required=True)`. https://docs.python.org/2.7/library/argparse.html#required
> I know, I was just pointing out that I find it a little awkward to use "optional" arguments as required arguments which is what "positional" is typically used for, but that's just my personal preference. I'm fine changing that if you prefer.
I don't understand why you say they're optional arguments.. To me, they are "named" arguments. Optional is when `required=False`, mandatory is when `required=True`. But regardless, I do think that using named arguments will be much better because of the self documentation -- I don't want to have to read this script to figure out what the command in the CMake is doing.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits