Closed GoogleCodeExporter closed 9 years ago
GoogleCodeExporter
commented
9 years ago I'd love to have this fixed, but I don't think we should prioritize it over
functional problems.
Do you need this for infrastructure reasons (e.g. automated packaging, etc) or
is it a cosmetic problem?Original comment by kim.gras...@gmail.com on 30 Aug 2013 at 12:16
GoogleCodeExporter
commented
9 years ago I am using the output of include-what-you-use --version to generate the
manpage. However, I have overridden the output. It is really minor...Original comment by sylvestre.ledru on 2 Sep 2013 at 7:35
GoogleCodeExporter
commented
9 years ago OK, thanks for the detail. I understand the necessity better now.
I'll leave it be for now, but it'd be great if we could get this fixed
longer-term.Original comment by kim.gras...@gmail.com on 2 Sep 2013 at 6:41
GoogleCodeExporter
commented
9 years ago Here's a patch that picks up the SVN rev in CMake, and passes it on to the IWYU
build.
Sylvestre, is this good enough version information for you?
Volodymyr, are you happy with the approach?Original comment by kim.gras...@gmail.com on 19 Jan 2014 at 8:44
Attachments:
GoogleCodeExporter
commented
9 years ago Hmmm, will it work when not built from the svn tree?
Original comment by sylvestre.ledru on 23 Jan 2014 at 12:13
GoogleCodeExporter
commented
9 years ago It should work for out-of-tree builds, as in:
https://code.google.com/p/include-what-you-use/wiki/InstructionsForUsers#Buildin
g_out-of-tree
(I'll have to test this.)
But if IWYU doesn't have its svn working copy it won't pick up the svn rev., is
that what you meant?
It's also only implemented for the CMake build, but I have a vague idea that
should work for the Make workflow as well.
Original comment by kim.gras...@gmail.com on 23 Jan 2014 at 5:01
GoogleCodeExporter
commented
9 years ago To my mind there is no need to provide short option -V, it is confusing. And
there is a little problem that we don't handle -v. By the way, we don't handle
-help too, only --help. I think it's a nice opportunity to evaluate if llvm::cl
is suitable for parsing IWYU options.
As you've already mentioned, Makefile support is required too.
I offer version string to be
include-what-you-use 0.2 (520) based on clang 3.5 (199888)
I offer to move (r520) earlier, so it is clear that it corresponds to IWYU, not
to clang. Use revision number without 'r' for consistency with clang itself.
Use 'based on' instead of 'built on' because Apple's clang uses 'based on', for
example, "Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)". I
don't insist on specifying clang revision number, but clang revision number in
bug reports will be helpful. Use own IWYU versioning because it looks weird
that IWYU doesn't have explicit versioning: either the same as LLVM+Clang or
separate. And separate versioning allows to release IWYU separately from
LLVM+Clang which is useful for bugfix releases.Original comment by vsap...@gmail.com on 24 Jan 2014 at 12:00
GoogleCodeExporter
commented
9 years ago Thanks!
As far as I can see, we do handle -v, it's the short alias for --verbose.
Python uses -V for version, so I figured it was a better short alias/option
value than "e".
I looked into llvm::cl a while back, but the whole Opt framework didn't really
agree with me. Depending on which LLVM/Clang libraries are linked, options
magically appear from nowhere, so we'd have to do filtering of the active
options. There's also the convention with tool/clang options being separated by
"--" instead of using a "-X" prefix, which messes up IWYU's ability to run as
part of a make build. I discussed this at length on the Clang list, but we
never came to any clear conclusion. I think I promised a patch, but I haven't
gotten around to it.
I was reluctant to explicit IWYU versioning because we're more or less forced
to follow Clang releases pretty closely anyway. But it makes more sense,
especially to start at < 1.0. And you make a good point that we can release
more often.
The version string you propose looks fine, I'll go back and see what is doable.
Still curious about Sylvestre's "Hmmm, will it work when not built from the svn
tree?" question, could you explain more?Original comment by kim.gras...@gmail.com on 25 Jan 2014 at 9:38
GoogleCodeExporter
commented
9 years ago I mean that "iwyu -v" prints Clang version and "iwyu -Xiwyu -V" isn't very
useful (len("--version") == len("-Xiwyu -V" == 9). I've investigated that
currently we cannot provide IWYU version for verbose output ("-v" flag) without
Clang changes, so for now will print Clang version (see FIXME in
Driver::PrintVersion).
I've looked into llvm::cl too and think it is unsuitable for our purposes. It
doesn't handle Clang options, doesn't provide custom error handling, and
doesn't support current implementation nicely (--help output, --long_option vs
-long_option). So we'll use our own options' parsing.
What if we separate IWYU-specific options from intercepted options? So that
"-Xiwyu --help", "-Xiwyu --version" are disallowed? I've attached a patch with
such approach.
By the way, clang::getClangRevision() works with "make" but doesn't work with
cmake+ninja.Original comment by vsap...@gmail.com on 26 Jan 2014 at 3:08
Attachments:
GoogleCodeExporter
commented
9 years ago Ah, I completely overlooked the -Xiwyu prefix in relation to -V.
Clang uses part of the llvm::cl library, just not the declarative framework, so
I think we could do the same. But it takes some thinking on how to set up an
option table, and how to have it interact with Clang's ditto. So for now I
think our own options parsing is fine.
It's a little jarring that we put so much machinery in place for something as
simple as options parsing :-/
I think your patch makes it clearer what's going on, but maybe we should
consider catching "-h" and "-v" in main() just like "--help" and "--version"
and wire them up as shortopts in ParseInterceptedCommandlineFlags?
Clang's Version.cpp seems broken under cmake/ninja, the revision number ends up
returned from clang::getClangRepositoryPath(). I'll ask on cfe-dev.Original comment by kim.gras...@gmail.com on 26 Jan 2014 at 4:15
GoogleCodeExporter
commented
9 years ago I don't want to intercept "-v" because in case of "clang -v some_file.cc" Clang
prints invocation information, header search paths. To my mind it's useful and
I'd like to keep this functionality despite non-printing IWYU version.
As for "-h" option I don't want to use it because Clang might start using it
for some other purpose.
I have found how SVN revision is provided to Clang in Makefile build and have
done the same for IWYU. Also I've renamed IWYU_SVN_REV to IWYU_SVN_REVISION to
be more consistent with Clang.
File iwyu_version.h is introduced to store IWYU version. It is very simple and
straightforward mechanism. Feel free to suggest something better.
Also I've introduced class OptionsParser to keep code responsible for 'which
options are intercepted' and 'how to handle intercepted options' together. What
do you think about this approach?Original comment by vsap...@gmail.com on 31 Jan 2014 at 4:42
Attachments:
GoogleCodeExporter
commented
9 years ago Ah, good point about "-v", I didn't know it was dual-purposed. Now I also
understand your FIXME comment on ParseInterceptedCommandlineFlags better.
Overall, I like this a lot, it gives IWYU a private versioning scheme, and SVN
information is nicely included automatically.
I feel that introducing a class for options parsing is a little heavy-handed,
but I see that it makes memory management easier. I think I would prefer it if
it was a function that returned clang args to pass into CreateCompilerInstance,
something like:
std::vector<char*> ParseIwyuCommandline(int argc, char** argv) {
// Split out intercepted args (--help, --version)
// Split out IWYU args (-Xiwyu ...)
// Push all others onto vector clang_args
// Handle intercepted args
// Handle IWYU args
return clang_args;
}
But it's not super-important for this change, I can send a refactoring patch
later if it bothers me too much :-)
iwyu.cc:
// This code has various memory leaks, but we're in main, so who cares?
This comment in main() now seems outdated.
iwyu_globals.cc:
+static void PrintVersion() {
+ // Build version string.
+ string buffer;
+ llvm::raw_string_ostream ostream(buffer);
+ ostream << "include-what-you-use " << IWYU_VERSION_STRING;
+#ifdef IWYU_SVN_REVISION
+ string iwyu_svn_revision = IWYU_SVN_REVISION;
+#else
+ string iwyu_svn_revision = "";
+#endif
+ if (!iwyu_svn_revision.empty()) {
+ ostream << " (" << iwyu_svn_revision << ")";
+ }
+ ostream << " based on clang " << CLANG_VERSION_STRING;
+ string clang_svn_revision = clang::getClangRevision();
+ if (!clang_svn_revision.empty()) {
+ ostream << " (" << clang::getClangRevision() << ")";
+ }
+
+ // Print version string.
+ llvm::outs() << ostream.str() << "\n";
+}
Is there any reason to buffer the output? Also, it turns out that Clang has a
library function that gives you "clang version X.Y (nnnnnnn)":
getClangFullVersion(). I propose:
static void PrintVersion() {
// Print version string.
llvm::outs() << "include-what-you-use " << IWYU_VERSION_STRING;
#ifdef IWYU_SVN_REVISION
llvm::outs() << " (" << IWYU_SVN_REVISION << ")";
#endif
llvm::outs() << " based on " << clang::getClangFullVersion();
llvm::outs() << "\n";
}
+ iwyu_argv[iwyu_argc] = NULL; // argv should be NULL-terminated
+ intercepted_argv[intercepted_argc] = NULL;
+ clang_argv_[clang_argc_] = NULL;
The trailing comment now applies to all three lines, so move it to its own line.
Makefile:
+
+CPP.Defines += -DIWYU_SVN_REVISION='"$(IWYU_SVN_REVISION)"'
Line before CPP.Defines has trailing whitespace.
I don't know Make syntax, so I assume the single-quotes around the
dobule-quotes are necessary. It looks a little over-quoted, but I'm guessing
there's a reason.Original comment by kim.gras...@gmail.com on 9 Feb 2014 at 3:23
GoogleCodeExporter
commented
9 years ago Oh, and I forgot: --version should be listed in PrintHelp.Original comment by kim.gras...@gmail.com on 9 Feb 2014 at 3:28
GoogleCodeExporter
commented
9 years ago Kim, thanks for the feedback. All your comments make sense, will update the
patch accordingly.Original comment by vsap...@gmail.com on 10 Feb 2014 at 8:53
GoogleCodeExporter
commented
9 years ago A new patch is attached.
Regarding a class for options parsing, I've also looked into how to add support
for compilation database. Currently I am inclined to use our own options
parsing and ClangTool, which requires compilation database and source paths. In
this case a class is more suitable than a function, I think.
There was still 1 memory leak in main(). I've fixed it and removed the comment
about memory leaks.
> [...] PrintVersion [...] Is there any reason to buffer the output?
No reason, at some point I had locally GetVersion and PrintVersion. Buffering
the version string is just remains of GetVersion. getClangFullVersion is good
idea. I've left a check "if (!iwyu_svn_revision.empty())" because when there is
no SVN info, IWYU_SVN_REVISION is defined, but equals to "".
> I don't know Make syntax, so I assume the single-quotes around the
double-quotes are necessary.
Single quotes are used so that make doesn't consume double quotes. This way
double quotes are present in the source code when macro is expanded.
Also I've tuned help/usage message and a few comments.Original comment by vsap...@gmail.com on 19 Feb 2014 at 2:20
Attachments:
GoogleCodeExporter
commented
9 years ago +#ifdef IWYU_SVN_REVISION
+ string iwyu_svn_revision = IWYU_SVN_REVISION;
+#else
+ string iwyu_svn_revision = "";
+#endif
I would prefer if the build failed if IWYU_SVN_REVISION wasn't defined, so you
can just skip the #ifdef. Now that both the CMake and Make systems
unconditionally defines it (maybe to an empty string, but it's still defined),
I see no reason to allow building without it.
+#define IWYU_VERSION_STRING "0.2"
Please add include guards in iwyu_version.h
Other than that, this looks great!Original comment by kim.gras...@gmail.com on 22 Feb 2014 at 9:46
GoogleCodeExporter
commented
9 years ago Committed r526 with changes according to review. Thanks, Kim.
Sylvestre, can you please confirm that the new version string is suitable for
your purposes?Original comment by vsap...@gmail.com on 23 Feb 2014 at 12:38
GoogleCodeExporter
commented
9 years ago Oops, I'm now hitting an assert in llvm/include/llvm/ADT/SmallVector.h line 144
with this patch applied on Windows.
When I revert back to r525 everything runs fine again. I'll try and debug
through it to find the root cause.Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 9:51
GoogleCodeExporter
commented
9 years ago What is the current display? (I cannot build iwyu right now)
ThanksOriginal comment by sylvestre.ledru on 23 Feb 2014 at 10:38
GoogleCodeExporter
commented
9 years ago False alarm! After cleaning and rebuilding, I don't see this anymore.
Disconcerting, but unrelated to this patch. Sorry about the noise.Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 10:50
GoogleCodeExporter
commented
9 years ago @sylvestre:
$ include-what-you-use.exe --version
include-what-you-use 0.2 (527) based on clang version 3.5 (201969)
We've introduced a proper version number for IWYU to better reflect its
maturity.Original comment by kim.gras...@gmail.com on 23 Feb 2014 at 10:51
GoogleCodeExporter
commented
9 years ago Sounds perfect! Thanks!
Original comment by sylvestre.ledru on 23 Feb 2014 at 10:52
Original issue reported on code.google.com by
sylvestre.ledruon 30 Aug 2013 at 7:35