Precompiled-headers support with --prefix_header_includes?

[GoogleCodeExporter]

I am trying to figure out if there is any support for precompiled headers with 
iwyu. The only thing I found is to provide the precompiled header to clang with 
-include and --prefix_header_includes with to iwyu. However the results are not 
what I expected.

For the test I am attaching, the command line reads like this:

F:\tests\build\bin\Release\include-what-you-use.exe 
F:\tests\iwyutest\iwyutest\iwyutest.cpp -w -MD -I"C:\Program Files 
(x86)\Microsoft Visual Studio 10.0\VC\include" -IF:\tests\iwyutest 
-IF:\tests\iwyutest\iwyutest -DWIN32 -DNDEBUG -D_CONSOLE -D_UNICODE -include 
F:\tests\iwyutest\iwyutest\stdafx.h -Xiwyu --prefix_header_includes=remove

And the result is this:

F:/tests/iwyutest/iwyutest/iwyutest.cpp should add these lines:
#include "tchar.h"                      // for _TCHAR, _tmain

F:/tests/iwyutest/iwyutest/iwyutest.cpp should remove these lines:
- #include "stdafx.h"  // lines 4-4

The full include-list for F:/tests/iwyutest/iwyutest/iwyutest.cpp:
#include "tchar.h"                      // for _TCHAR, _tmain

Both suggestions seem incorrect right?

1) #include "tchar.h" should not be necessary because it's already included in 
stdafx.h
2) stdafx.h is needed because this is a precompiled header (although I can 
understand that if there is no real support for this it can complain about it)

Am I doing something wrong here?

Original issue reported on code.google.com by dpun...@gmail.com on 10 Apr 2014 at 2:58

Attachments:

• iwyutest.rar

[GoogleCodeExporter]

Here's an updated patch.

I struggled a little with the test case, let me know if there's anything I can 
do to make it clearer.

Original comment by kim.gras...@gmail.com on 22 Apr 2014 at 7:55

Attachments:

• 126r2.patch

[GoogleCodeExporter]

My comments:

Please express in test more clearly that it's not for all precompiled headers, 
but a special mode for codebases which follow such style.

new_file is referenced in comment in IwyuPreprocessorInfo::AddDirectInclude, 
but new_file exists in IwyuPreprocessorInfo::FileChanged_EnterFile.  Seems like 
set_pch_in_code was at first in FileChanged_EnterFile, why have you moved it (I 
don't know which one is better, just asking)?

Assert in GetSortKey is unnecessary.  If fails on good old placement operator 
new in template.  Not a problem if file_info is NULL because it is so only when 
there was no <new> in code and not-in-code #include cannot be pch-in-code.

Not sure if added comments in GetSortKey are useful. You can figure out the 
appropriate category from int.

In PrintHelp the style is "--pch_in_code: <lowercase letter>..."

In PrintHelp --pch_in_code description isn't very good.  I think we need to 
describe more a visible effect and why user should use it.  Now it describes 
how we treat the option in code, which is pretty useless for users.  I like a 
comment for IwyuFileInfo::is_pch_in_code_, think we can use some of that text.

Original comment by vsap...@gmail.com on 23 Apr 2014 at 9:23

[GoogleCodeExporter]

Thanks for review!

The pch_in_code check moved from FileChanged_EnterFile to AddDirectInclude out 
of necessity -- MaybeProtectInclude is called at the end of AddDirectInclude, 
and it needs to know if the include to be protected is a pch or not.

Revised patch attached.

One thing occurred to me: the pch_in_code flag is really a quality of the 
OneIncludeOrForwardDeclareLine that represents whether _that specific line_ is 
the PCH-in-code #include. That would eliminate the extra reverse lookup for 
GetSortKey, and we could check the line object directly.

Unfortunately we also need the information to know if we should protect the 
include (MaybeProtectInclude) and if all its includes should be considered 
prefix headers (FileChanged_EnterFile). Those checks are all keyed on the 
IwyuFileInfo.

Please take a look at this patch, and I'll investigate alternative solutions 
for putting pch_in_code on OneIncludeOrForwardDeclareLine.

Original comment by kim.gras...@gmail.com on 24 Apr 2014 at 7:40

Attachments:

• 126r3.patch

[GoogleCodeExporter]

I understand now why the code is in AddDirectInclude, not in 
FileChanged_EnterFile. I suggest to change comment in AddDirectInclude

-// Now we know new_file is the first included header file. Mark it as
+// Now we know includee is the first included header file. Mark it as

Investigating how to put pch_in_code on OneIncludeOrForwardDeclareLine and what 
will be the result is interesting. It might make something harder and something 
easier, hard to tell what will be the final result.

Looks like I prefer long names and you prefer more vertical space, but our 
personal preferences aren't widespread in IWYU codebase. I think code in 
GetSortKey is short enough not to benefit from extra empty lines.

My take on help message (doesn't mean you should incorporate it verbatim, just 
think that a concrete example can be more helpful than vague wishes and 
recommendations):

--pch_in_code: mark the first #include in a translation unit as a precompiled 
header.  IWYU will keep this header as the first #include and will keep it even 
if it's not used.  PCH-in-code mechanism can be used with GCC and is standard 
practice on MSVC, whereas Clang forces PCHs to be listed as prefix headers.  It 
is recommended to use --pch_in_code option when IWYU breaks PCH mechanism by 
interfering with precompiled header #include.

I'm not very happy with my version too, will try to improve.

Original comment by vsap...@gmail.com on 25 Apr 2014 at 8:23

[GoogleCodeExporter]

Thanks!

Apparently my last edits were sloppy. I meant to change the comment and restore 
GetSortKey to the way it looked before, but I seem to have got lost in some 
other edits.

Attached is a new rev, and I tried to improve the help message slightly as well 
to have it say when it might be useful.

Original comment by kim.gras...@gmail.com on 26 Apr 2014 at 7:45

Attachments:

• 126r4.patch

[GoogleCodeExporter]

I gave up on the OneIncludeOrForwardDeclareLine adjustment, it ultimately 
turned out more complicated. Please take a look at the latest posted patch.

Original comment by kim.gras...@gmail.com on 26 Apr 2014 at 7:31

[GoogleCodeExporter]

One more case: when we have --pch_in_code, -include=pch.h, 
--prefix_header_includes=remove.  In this case I want to keep pch.h and I've 
added corresponding check to CleanupPrefixHeaders.  Also I've noticed that 
pch.h wasn't prefix header but all included headers were.  Probably it helped 
to keep pch.h in face of --prefix_header_includes=remove, but it was doing this 
not very reliably, so I think it will be more consistent to make pch.h prefix 
header.  Changes involve 2 files, corresponding patch is attached (it's patch 
on top of trunk, not on top of your last patch).

Regarding the help message.  I think the sentence explaining PCH-in-code isn't 
very useful.  I might be wrong, but I wanted to explain what prefix header is, 
because there is no single term for such headers.  And "precompiled header" 
term seems to be widely known and I don't know other terms for precompiled 
headers.  I suggest text

--pch_in_code: mark the first include in a translation unit as a precompiled 
header.  Use --pch_in_code to prevent IWYU from removing necessary PCH include. 
 Though Clang forces PCHs to be listed as prefix headers, PCH-in-code pattern 
can be used with GCC and is standard practice on MSVC (e.g. stdafx.h).

Sentences' roles: 1) say what the option does; 2) why user should care about 
the option, visible option's impact; 3) option justification with keywords 
relevant to anticipated problems with MSVC-compatible codebases.

Original comment by vsap...@gmail.com on 27 Apr 2014 at 12:55

• Changed state: Accepted

Attachments:

• issue126-is-removable-prefix.patch

[GoogleCodeExporter]

Thanks!

> One more case: when we have --pch_in_code, -include=pch.h,
> --prefix_header_includes=remove.  In this case I want to
> keep pch.h and I've added corresponding check to
> CleanupPrefixHeaders.

It's interesting, because if we have pch.h on the -include line, we don't need 
it in code. On the other hand, if we have a portable codebase and the Clang 
build happens to pass the PCH on the command-line but all other compilers use 
pch-in-code, we don't want IWYU to break them. I think it's a good idea to add 
this rule.

> Also I've noticed that pch.h wasn't prefix header but all included headers
> were.  Probably it helped to keep pch.h in face of
> --prefix_header_includes=remove, but it was doing this not very reliably,
> so I think it will be more consistent to make pch.h prefix header. 

I was reluctant to make pch.h a prefix header, because it's not. But then 
again, neither are the headers it includes, really.

> --pch_in_code: mark the first include in a translation unit as a
> precompiled header.  Use --pch_in_code to prevent IWYU from [...]

Works for me, I'll integrate it and commit your patch!

Original comment by kim.gras...@gmail.com on 27 Apr 2014 at 8:18

[GoogleCodeExporter]

Fixed in r539.

Original comment by kim.gras...@gmail.com on 27 Apr 2014 at 8:36

• Changed state: Fixed