search

HaveF / include-what-you-use

Automatically exported from code.google.com/p/include-what-you-use
Other
0 stars 0 forks source link

Option to not suggest inclusion of files provided on the commandline #112

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Repro:

1. Unzip the three files attached to this issue (main.cpp, pch.h, required.h)
2. Attempt to compile with the following command line (where $IWYU is your IWYU 
binary): 

    $(IWYU) -include pch.h main.cpp

Output:

main.cpp should add these lines:
#include "required.h"                   // for Required

main.cpp should remove these lines:

The full include-list for main.cpp:
#include "required.h"                   // for Required

---

Expected:

(main.cpp has correct #includes/fwd-decls)

The expected output for this would be for it to not suggest any additions, as 
the command line specifies an include which in turn includes required.h

Version:

I'm using the latest SVN tree, built in tree with LLVM and Clang on about 
October 1st 2013. I'm on Windows and I built IWYU with Visual Studio 2010.

Additional info:

This has been discussed briefly on the Google Group: 
https://groups.google.com/forum/#!topic/include-what-you-use/RLOjk7rRhGY

Original issue reported on code.google.com by max.dyck...@gmail.com on 5 Oct 2013 at 12:00

Attachments:

GoogleCodeExporter commented 9 years ago 
IWYU seems to just be doing its thing: replacing an indirect #include of 
required.h with a direct #include.

I'm not sure it should treat an #include differently just because it's on the 
command-line. But it comes down to intent/preference in a lot of ways, per our 
earlier discussion. I wonder if you could make that intent explicit, somehow?

Why is it that you want to avoid the direct includes?

Original comment by kim.gras...@gmail.com on 14 Oct 2013 at 5:08

GoogleCodeExporter commented 9 years ago 
Yes, it does seem to be doing the correct thing as designed. However as 
discussed, we will be building with that include file on the command line no 
matter what, and indeed with a couple of dozen others too. Some of these are 
very high level SDK includes that we have no interest in directly including 
ever, they may have crazy dependencies that we don't want to worry about. 
Others have #error directives that make sure they are never included directly 
for one reason or another. (The joys of C++ programming!)

But ultimately, there are probably at least a dozen includes that all of our 
cpp files rely on, which we just don't want to have to individually include in 
every single file.

Thoughts on the matter?

Original comment by max.dyck...@gmail.com on 14 Oct 2013 at 11:54

GoogleCodeExporter commented 9 years ago 
From a completely objective viewpoint (;-)) I think your code might be better 
off with the #includes explicit in source code.

But I've been considering using command-line includes to force stuff like a 
config.h or precompiled headers into every translation unit, so I see how you 
think it makes sense to treat them as sacred.

I've been experimenting with different ways of excluding command-line includes 
from IWYU analysis, but I still haven't found a good way forward.

Original comment by kim.gras...@gmail.com on 20 Oct 2013 at 7:22

GoogleCodeExporter commented 9 years ago 
Yeah we are using it for the precompiled header, and also for some really 
obscure PS3/4 SDK includes. The SDK includes also have a really complicated 
include path which is annoying; most of our includes are just two or three 
directories deep but the nature of our code layout means the SDK includes are a 
dozen or more directories away.

I'm sure we could fix it so they were easier to include, but even then you 
don't want to have to remember what obscure SDK file needs to be included to 
get things like SIMD etc.

I tried to put all the command line included files into a single .h and include 
that, with the pragma begin_exports, but it didn't seem to work and still 
forced me to include every file. I may have been doing something wrong, I only 
spent a couple of minutes trying it. :)

If I can get that to work, I can just allow it to suggest that I include 
"command_line_includes.h", and then filter it out.

Original comment by max.dyck...@gmail.com on 26 Oct 2013 at 7:19

GoogleCodeExporter commented 9 years ago 
I think you make a good case for this and we should consider allowing override 
of the default behavior for command-line includes.

I won't be able to get to it for a while, but I'll keep mulling on it.

The begin_exports/end_exports pragmas should help here, but there could be some 
additional complication that I'm missing.

Original comment by kim.gras...@gmail.com on 3 Nov 2013 at 10:50

GoogleCodeExporter commented 9 years ago 
I had an idea to add a command line option --external_includes=add|keep|remove. 
Where 
  'add' corresponds to the current behavior; 
  with 'keep' IWYU doesn't add new includes/forward declarations if they are made obsolete by command line includes, but keeps existing lines intact; 
  with 'remove' IWYU removes existing includes/forward declarations and doesn't add new if they are made obsolete by command line includes.

'add' is a default value. We believe it is "the right thing" and try to impose 
it on developers. 'keep' is what Max is asking for. And 'remove' is for 
completeness and additional flexibility as an opposite for 'add'.

Any comments? Preliminary patch is ready, I'll clean it up and publish for 
review later.

Original comment by vsap...@gmail.com on 6 Dec 2013 at 2:16

GoogleCodeExporter commented 9 years ago 
That sounds great! I think we would probably actually use the 'remove'.

Thanks!
 On Dec 6, 2013 6:16 AM, <include-what-you-use@googlecode.com> wrote:

Original comment by max.dyck...@gmail.com on 6 Dec 2013 at 4:01

GoogleCodeExporter commented 9 years ago 
Sounds excellent to me!

Thanks for picking this up, I've been busy with other things for a while, but I 
think I'm getting back to IWYU soon.

Looking forward to seeing the patch.

Original comment by kim.gras...@gmail.com on 6 Dec 2013 at 4:31

GoogleCodeExporter commented 9 years ago 
The overall idea is to keep a track if file is externally or in-source 
included. Then in the end we go through includes and forward declarations and 
ignore those which are superseded by external includes.

Original comment by vsap...@gmail.com on 8 Dec 2013 at 5:14

Attachments:

GoogleCodeExporter commented 9 years ago 
I like this, really nicely done!

My only high-level concern is the name "external includes", I find "external" a 
slightly too general term. I've tried to find an well-established term (MSVC 
calls them "forced includes", GCC doesn't call them anything in the docs for 
-include), but the closest I've found so far is "prefix header" [1]. It seems 
intimately involved with precompiled headers in XCode, but people appear to use 
it more generally for these command-line headers. I like the term, what do you 
think?

Also, the command-line options (keep, add, remove) seem a little backward to me 
-- they don't say anything about how to treat the _external includes_, but 
anything that _conflicts_ with them.

My only concrete code comment, otherwise, is:

+void CleanupExternalIncludes(vector<OneIncludeOrForwardDeclareLine>* lines,
+                             const IwyuPreprocessorInfo* preprocessor_info) {

The arguments should switch place here, |lines| is an in/out argument, and 
should come after the input-only preprocessor_info:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_
Parameter_Ordering#Function_Parameter_Ordering

[1] https://www.google.com/search?q=prefix+header

Original comment by kim.gras...@gmail.com on 9 Dec 2013 at 9:07

GoogleCodeExporter commented 9 years ago 
I like the term "prefix header".  What about 
--prefix_header_includes=add|keep|remove?  I've also considered 

  --prefix_header=add_includes|keep_includes|remove_includes.  It seems to be more correct, but I don't like *_includes repetition.  And here "includes" means "includes and forward declarations".  Forward declarations aren't mentioned explicitly, but only implied.

  --prefix_header=add|keep|remove.  It is short, but looks misleading to me.  I have an impression that this option turns prefix headers' mechanism on/off.

Also in help we call '-include foo.h' 'prefix header command-line include', 
'#include "foo.h"' 'prefix header in-source include', and 'foo.h' 'prefix 
header'.

I've changed arguments' order in CleanupExternalIncludes, thanks for the catch.

Original comment by vsap...@gmail.com on 10 Dec 2013 at 3:59

GoogleCodeExporter commented 9 years ago 

> --prefix_header=add|keep|remove.  It is short, but looks misleading to me.
> I have an impression that this option turns prefix headers' mechanism on/off.

Right, my intuitive reaction was the same.

How about if we phrase it in terms of how to treat prefix headers (just 
brainstorming here):

  --prefix_headers=ignore|use|force

It's less clear, but I think it conveys the same semantics. Maybe there are 
better words to use for the enumerators to make this work?

It's a shame add|keep|remove is so nice! :-)

> Also in help we call '-include foo.h' 'prefix header command-line include',
> '#include "foo.h"' 'prefix header in-source include', and 'foo.h' 'prefix 
header'.

Sorry, I don't understand what you're saying here. As far as I can see; 
anything that comes from the command-line is a prefix header, anything else 
isn't.

Original comment by kim.gras...@gmail.com on 11 Dec 2013 at 5:22

GoogleCodeExporter commented 9 years ago 
> --prefix_headers=ignore|use|force

I think it doesn't convey that IWYU modifies includes of prefix headers and 
doesn't touch prefix headers at all. I am afraid, it is impossible to express 
with option --prefix_headers=<verb> that not prefix headers are acted on, but 
related includes and forward declarations are modified.

> Sorry, I don't understand what you're saying here. As far as I can see; 
anything that comes from the command-line is a prefix header, anything else 
isn't.

I mean that naming all involved pieces can help to avoid confusion. For 
example, what does "external include" mean? Is it included file itself or 
inclusion directive or command-line option? But when we call
  included file itself - prefix header
  -include foo.h       - prefix header command-line option
  #include "foo.h"     - prefix header include
then we can use --prefix_header_includes and it is clear that it is about 
'#include "foo.h"'.

Original comment by vsap...@gmail.com on 11 Dec 2013 at 2:49

GoogleCodeExporter commented 9 years ago 
This works fantastically. I finally had the time to apply the patch and 
rebuild, and on the half dozen files in our codebase that I tried it on the 
results were far more like what I would expect. There are a couple of strange 
includes that I wouldn't expect, but that's probably something on our end, not 
your's! 

Thanks, I love this tool. I have no opinion on how it should be named. :)

Original comment by max.dyck...@gmail.com on 11 Dec 2013 at 5:11

GoogleCodeExporter commented 9 years ago 
max.dyckhoff: thanks for verifying! I don't blame you for leaving the more 
important discussion to us :-)

vsapsi: thanks, now I see what you're saying! I think "prefix header include" 
is too ambiguous, I see why you suggested "in-source" earlier.

I'm thinking in these terms:

  -include foo.h    prefix include
  #include "foo.h"  prefix include overlap

The header itself isn't important here, only the overlap between what's forced 
on the command-line ("prefix include") and what's written in source code 
("prefix include overlap".)

I'm not super happy with "overlap", but I think it works, and we could even 
shorten it to "--prefix_overlap=add|keep|remove".

What do you think?

Original comment by kim.gras...@gmail.com on 12 Dec 2013 at 7:44

GoogleCodeExporter commented 9 years ago 
I think we shouldn't omit "header" from "prefix header".  "Prefix" is widely 
used on its own, that's why I don't expect "prefix" to be immediately 
associated with "prefix header".

I don't like --prefix_overlap/--prefix_include_overlap because it is hard to 
guess what does it mean without reading help.  On the other hand it is easier 
to guess what --prefix_header_includes means, though it is possible to guess 
incorrectly.

Currently I prefer --prefix_header_includes.  I want to believe that possible 
confusion between in-source includes and command-line options isn't very 
important.  IWYU adds and removes in-source includes, so I think it is 
reasonable to expect that --prefix_header_includes=add adds in-source includes, 
not includes via command-line option.

Original comment by vsap...@gmail.com on 13 Dec 2013 at 12:20

GoogleCodeExporter commented 9 years ago 
OK, we disagree about the finer details but I think all suggestions have their 
strengths and weaknesses. Let's go with yours: 
  --prefix_header_includes=add|keep|remove

It'd be nice to get this into trunk. LLVM/Clang 3.4 is approaching, and I think 
we're in a between-state right now, Everything but r495 is 3.4-compatible, so 
we may have to cherry-pick a little into a 3.4 branch to get a consistent state.

Original comment by kim.gras...@gmail.com on 14 Dec 2013 at 9:28

GoogleCodeExporter commented 9 years ago 
I've updated the patch with --prefix_header_includes. I've removed the short 
option because I haven't found a good one. If necessary, we can add short 
option later.

> It'd be nice to get this into trunk. LLVM/Clang 3.4 is approaching, and I 
think we're in a between-state right now, Everything but r495 is 
3.4-compatible, so we may have to cherry-pick a little into a 3.4 branch to get 
a consistent state.

Thanks for the reminder about differences about trunk and 3.4 branch. I think 
we can branch from r494 and then merge everything except of r495 from trunk to 
branch.

Original comment by vsap...@gmail.com on 14 Dec 2013 at 6:06

Attachments:

GoogleCodeExporter commented 9 years ago 
Patch looks good to me. I've built and run the tests on Windows, and all is 
well. Feel free to commit!

> I think we can branch from r494 and then merge everything except of r495
> from trunk to branch.

Yes, that should work. I'll try to get a 3.4 branch set up here, so I can test.

Original comment by kim.gras...@gmail.com on 14 Dec 2013 at 11:00

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r497.

Original comment by vsap...@gmail.com on 15 Dec 2013 at 12:33

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r500.

Original comment by vsap...@gmail.com on 15 Dec 2013 at 2:08