iwyu should test the include s

[GoogleCodeExporter]

Let's take the following file:

---
#include <glib.h>
#include <stdio.h>

int main() {
  char* f = g_strdup_printf("%s", "test");
  printf("%s\n", f);
  g_free(f);
  return 0;
}

---

iwyu recommends:
$ iwyu `pkg-config --cflags glib-2.0` foo.c 
[...]

The full include-list for foo.c:
#include <stdio.h>                      // for printf
#include "glib/gmem.h"                  // for g_free
#include "glib/gstrfuncs.h"             // for g_strdup_printf

If I rewrite the file as suggested, I get:

---
#include <stdio.h>                      // for printf
#include "glib/gmem.h"                  // for g_free
#include "glib/gstrfuncs.h"             // for g_strdup_printf

  int main() {
    char* f = g_strdup_printf("%s", "test");
    printf("%s\n", f);
    g_free(f);
    return 0;
  }

---

which will fail to build with:

---
In file included from foo.c:2:
/usr/include/glib-2.0/glib/gmem.h:31:2: error: "Only <glib.h> can be included
      directly."
#error "Only <glib.h> can be included directly."
 ^

---

A way for iwyu to check if it works would be to include the header in a 
temporary file and build it.

Original issue reported on code.google.com by sylvestre.ledru on 9 Sep 2013 at 7:04

[GoogleCodeExporter]

IWYU generally attempts to solve this using mappings, see 
https://code.google.com/p/include-what-you-use/wiki/IWYUMappings.

But I wonder if we could create a soft rule that checks #error directives, 
something like "for any #error directive inside an #ifndef (or equivalent) 
where the message text contains the word 'include' and the pattern '*.h', 
create a mapping from current header to the mentioned quoted include name."

It seems to me that would get very few false positives and interpret the 
majority of these #include hints in third party libraries. There might need to 
be a way to turn this off, though, if it does trigger in error.

Thoughts?

Original comment by kim.gras...@gmail.com on 9 Sep 2013 at 9:11

[GoogleCodeExporter]

I have a crazy draft for this using regular expressions. I can't recommend it, 
primarily because it slows down IWYU incredibly (it's been a while since I saw 
my CPU so busy :-))

I thought I'd try and reimplement like ProcessHeadernameDirectivesInFile, based 
on simple strstr instead, and see if I can make it lenient enough.

It'd be interesting to hear if this solves the problem with glib, though...

Original comment by kim.gras...@gmail.com on 18 Aug 2014 at 9:16

Attachments:

• 109.patch

[GoogleCodeExporter]

Here's an update where I only have 1½ problem; most regexes are gone in favor 
of using Clang's raw lexer. I don't know if there are even nicer ways of lexing 
for preprocessor directives, but the immediate perf hit is gone anyway.

I'm still not sure this is a good idea, but it actually appears to work on a 
contrived case.

It'd be fun to hear from Sylvestre if his case with glib.h works with this 
patch.

Original comment by kim.gras...@gmail.com on 19 Aug 2014 at 8:35

Attachments:

• 109r2.patch

[GoogleCodeExporter]

I doubt it's the best approach. I was thinking about adding a mapping when 
includee uses a macro from includer. Pretty sure such heuristic will have false 
positives, need to check on real code. Sorry, I don't have a patch to 
demonstrate this approach. Hope to submit one soon.

I'm not sure my approach is better or even if it'll work. It just seems to be 
less hackish and worth to be investigated.

Original comment by vsap...@gmail.com on 25 Aug 2014 at 5:37

[GoogleCodeExporter]

Nice idea, though it will break for x-macros (issues #74 and #45).

I agree that my suggestion is a big ugly hack. But I think it's a pretty useful 
heuristic, and will work out of the box for many well-designed libraries.

Another pro is that libraries can become more IWYU-friendly by marking their 
private headers with a construct that's also useful outside of IWYU (an #error 
firing if you #include a private header can be thought of as general good 
hygiene.)

I'd like to add an opt-out command-line switch to make sure this heuristic can 
be turned off if it ever does fire in error.

But I fully understand if you think it's too brutal to be included :-)

Original comment by kim.gras...@gmail.com on 25 Aug 2014 at 6:45

[GoogleCodeExporter]

A quick and dirty patch to illustrate my idea is attached. Yes, it fails for 
x-macros. But I think that we can detect and handle x-macros in similar way. 
I.e. if includee uses macros defined in includer, then inclusion directive 
should be kept even if includee was included earlier.

Original comment by vsap...@gmail.com on 28 Aug 2014 at 4:37

Attachments:

• issue109-macro_defined_in_includer.patch

[GoogleCodeExporter]

Nicely done!

I don't really understand how any of the #include 
"tests/macro_mapping_heuristic-d2.h" are kept; I feel they should be mapped to 
macro_mapping_heuristic.cc and self-destruct :-)

It seems to have something to do with the logic in ProtectReexportIncludes, but 
I can't make heads or tails of it. It seems like this function does indeed 
protect the #include and overrides the mapping.

As you say, maybe the same technique could be used to whitelist all these 
includes for ClearDesiredForSurplusIncludesOrForwardDeclares.

I'm happy with this approach as long as x-macros is somehow solvable. The 
condition you're using to avoid private includes is exactly the same as what 
x-macros is based on, so I was worried that it would completely short-circuit 
any solution, but there may be workarounds (probably are, given that 
ProtectReexportIncludes somehow overrides the mapping.)

Original comment by kim.gras...@gmail.com on 28 Aug 2014 at 8:31

[GoogleCodeExporter]

I think #include "tests/macro_mapping_heuristic-d2.h" are kept because mappings 
are added only if !ShouldReportIWYUViolationsFor(used_in). It works more or 
less in case of main_file.cc and not_main_file.h, but main_file.h requires more 
work.

Original comment by vsap...@gmail.com on 29 Aug 2014 at 4:33