Closed GoogleCodeExporter closed 8 years ago
Looks like the best solution is here:
http://www.codeproject.com/KB/cpp/xgetopt.aspx. It's in the public domain, so
we can just incorporate it into iwyu (or clang, or llvm, or...)
Original comment by csilv...@gmail.com
on 12 Mar 2011 at 9:54
That looks promising, although on first inspection it appears to only support
getopt(), and not getopt_long(). I'll take a more detailed look and see how
much work would be required to add support for getopt_long.
Original comment by paul.hol...@gmail.com
on 13 Mar 2011 at 12:44
We could just support --name=<value> (and not even support short options) for
windows. Or just support short options, and change the unittest framework to
pass in short options instead of long. We probably don't need full flexibility
here.
Original comment by csilv...@gmail.com
on 14 Mar 2011 at 7:43
I've applied the temporary fix in r62. I'll leave this issue open whilst I
tackle adding getopt() support.
Original comment by paul.hol...@gmail.com
on 14 Mar 2011 at 9:47
I've finally gotten around to looking at this. Before I add the getopt()
support I wanted to create a port.cc for it to live in. How does the attached
patch look?
I've taken the opportunity to pull a couple of the Windows-specific headers out
of port.h which avoids cluttering the Windows build with too much unnecessary
cruft.
Original comment by paul.hol...@gmail.com
on 22 May 2011 at 10:32
Attachments:
I'm afraid port.cc is going to get pretty big if you put entire getopt
implementations in it. How about just creating a getopt.cc, and documenting
that it's only for windows (and maybe put an #idef _WIN32 guard in it)? I'm
guessing it's probably a mistake to force all os/cpu-specific stuff to live in
one .cc file, in any case.
Original comment by csilv...@gmail.com
on 24 May 2011 at 4:26
I've implemented this in the attached patch. There's a new getopt.cc which I
include in the CMakeLists.txt (and which I guess is picked up automatically by
the Makefile). It's got a _MSC_VER guard to match what we're using in port.h
I'm using the XGetOpt code as suggested above. It only supports the short
options for now, but that's sufficient to get up and running on Windows and at
least means IWYU will now compile out of the box. We can revisit longopt
support if required.
For reference, the short form of the flags must be specified like this:
path\to\iwyu -Xiwyu -v6 <clangargs>
Note '-v6' and not '-v=6'. I can fix this with a one-liner, but I'm not sure if
this is standard getopt() behaviour.
Finally, I moved the <getopt.h> include from the head of iwyu_globals.cc and
into port.h. This is consistent with how we're handling fnmatch.h, but I'm
happy to revert and guard with _MSC_VER if that's preferred.
Original comment by paul.hol...@gmail.com
on 24 May 2011 at 10:16
Attachments:
Overall the patch looks good.
We recently made a change so we no longer need to export fnmatch.h from port.h.
We should probably do the same thing here: provide a wrapper to getopt
(GetoptWrapper? :-) ) that we provide in port.h, and then the implementation in
port.cc either calls out to getopt.cc or #includes getopt.
But this may not be practical, given the need to export optarg and optind. So
let's do it this way instead:
call the new file iwyu_getopt.cc (instead of getopt.cc) to avoid confusion with
the libc getopt.cc. And let's provide iwyu_getopt.h, which for windows does
what you do in port.h, and for non-windows just does #include getopt.h.
That's the same as your patch now, just breaking this out so it's not part of
port.h anymore. I think for 'bigger' porting projects like this, it's ok to
have our own file.
Original comment by csilv...@gmail.com
on 25 May 2011 at 1:40
Sounds good. I've implemented the changes as suggested. It definitely seems a
bit neater like this.
Original comment by paul.hol...@gmail.com
on 25 May 2011 at 6:43
Attachments:
Looks good! Feel free to apply.
Original comment by csilv...@gmail.com
on 25 May 2011 at 6:47
Great! That's applied in r250.
I'll mark this issue as fixed. Although support for longopts is still missing,
I think this issue as raised is now fixed.
Original comment by paul.hol...@gmail.com
on 25 May 2011 at 7:00
Original issue reported on code.google.com by
paul.hol...@gmail.com
on 12 Mar 2011 at 4:31Attachments: