iwyu macros bug

[GoogleCodeExporter]

What steps will reproduce the problem?  Give the *exact* arguments 

I seem to be coming up against an iwyu problem with Macros

Let say we have 1 cpp and 1 header in test folder 
--> the cpp "Macro.cpp"

#define Vector_ bool
#define TYPE int 
#include "Macro2.h"
#undef TYPE
#define TYPE double 
#include "Macro2.h"
#undef TYPE
int main()
{   
    f(3);
    f(3.1);
}

--> the header "Macro2.h"

void f(TYPE const t) {
}

What is the expected output? What do you see instead?
I should see "correct includes", instead iwyu want to remove #include "Macro2.h"

What version of the product are you using? On what operating system?
iwyu r250

Please provide any additional information below.

Original issue reported on code.google.com by mehdi.ma...@gmail.com on 22 Jun 2011 at 5:14

[GoogleCodeExporter]

Hmm, thanks for the report.  If you're able to run this in a debugger, maybe 
you can figure out what's going on -- this doesn't seem like a hard case for 
iwyu to get right, to me.

If you do figure out what's wrong, I'm happy to accept a patch!

Original comment by csilv...@gmail.com on 22 Jun 2011 at 6:56

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

I've been debugging this a little, and it looks like the problem is not 
directly related to the use of macros per-se, but that the Macro2.h header is 
included 2 times and defines different symbols.

The actual output of IWYU is:

macro.cpp should add these lines:

macro.cpp should remove these lines:
- #include "Macro2.h"  // lines 3-3

The full include-list for macro.cpp:
#include "Macro2.h"                     // for f
---

I.e. it's trying to remove *one* of the includes of Macro2.h, but leaves the 
other. 

It appears that PrintableDiffs() is set up to handle duplicate includes, but it 
seems that only one entry in 'lines' is marked as is_desired_. Backtracking a 
little, one place I stepped through was 
CalculateDesiredIncludesAndForwardDeclares(). On entry to this function, 
'lines' has 2 entries:

lines:
[0] quoted_include_= "Macro2.h", start_linenum_ = 3, is_desired_ = false
[1] quoted_include_= "Macro2.h", start_linenum_ = 6, is_desired_ = false

include_map is initially constructed like this:
"Macro2.h" -> 1

As the function iterates over 'uses', it repeatedly calls set_desired() on 
lines[1]. This means that lines[0] is never marked as desired.

I don't really understand this part of the code well enough to know how to 
proceed. For this to work, OneUse would have to be able to carry around enough 
information to be able to discriminate between the two different includes of 
Macro2.h. i.e. OneUse::suggested_header() would need to return something other 
than a string, with enough info to map to a OneIncludeOrForwardDeclareLine when 
building include_map). This sounds like it could be quite a lot of work!

However, perhaps there is a much simpler solution to this which I've overlooked?

Original comment by paul.hol...@gmail.com on 16 Jul 2011 at 11:07

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Yes, that makes sense to me -- I can see that including a file twice would 
cause iwyu to get confused.

And in the common case, we *do* want to remove duplicates: at google we see 
code like this from time to time:
   #include "a.h"
   #include "b.h"
   ...
   <lots more includes>
   ...
   #include "b.h"

and want to get rid of the second b.h.  So iwyu would need to be aware that the 
two includes were different -- perhaps because there was some 'contentful' code 
in between them, and treat them appropriately.  That does seem like a lot of 
work.

There are two possible "easy" fixes that I see:

1) When calling set_desired(), make sure it iterates over all quoted-include 
lines and marks all lines with the proper name as 'desired'.  This would solve 
this problem, but would mean we no longer would get rid of duplicate b.h's.

2) Mark the Macro2.h lines with // IWYU pragma: keep.

I prefer solution #2. :-)

Original comment by csilv...@gmail.com on 18 Jul 2011 at 11:37

• Added labels: ****
• Removed labels: ****