IWYU and Static members

[GoogleCodeExporter]

There seems to be some issue on how to handle static members, when making the 
report. I have attached two different cases that do the same but through 
different ways.

Both samples will declare a static member inside a class by forward declaring 
the type. Then on the cpp of that class they will invoke some method on the 
static member.

1) Plain: In this sample we declare a static member directly inside 
ZMyTestClass by forward declaring the type. If you run iwyu notice how it 
suggests to remove the forward declaration and replace it by the include 
itself, while it says that the cpp is correct. It's not needed, right?

2) ThroughMacro: This sample is closer to what I have here. We have a header 
that defines some macros that are used in many classes. Some macros are used 
for the declaration and some used for the definition. In those cpp where 
definition macros are used we need to include the corresponding header (in this 
case, myutil.h). The report is suggesting to remove myutil.h, which doesn't 
compile.

What do you think?

Original issue reported on code.google.com by dpun...@gmail.com on 14 Apr 2014 at 7:54

Attachments:

• Plain.rar
• ThroughMacro.rar

[GoogleCodeExporter]

Thanks for the report!

The problem seems to be that CanForwardDeclareType returns false for your 
static member type use, and so the elaborated type check in VisitTagType is 
never reached.

This should be solvable, I'll try and look into it. But now it's time for bed, 
so it'll be a while.

Original comment by kim.gras...@gmail.com on 14 Apr 2014 at 9:00

[GoogleCodeExporter]

Awesome! Rest well!

Original comment by dpun...@gmail.com on 14 Apr 2014 at 9:08

[GoogleCodeExporter]

Here's a draft patch for this issue. I'll add some proper test cases if it 
appears to solve your problem.

Let me know how this works!

Original comment by kim.gras...@gmail.com on 16 Apr 2014 at 6:48

Attachments:

• issue127.patch

[GoogleCodeExporter]

Thank you! Actually I was starting to get a bit crazy trying to find the 
methods to know if a declared member was static :_). Now I see!

I tried it and this fixes the plain sample, but not the one with macros. That 
one is still suggesting to remove the header that is actually needed to get the 
implementation of the function.

Original comment by dpun...@gmail.com on 16 Apr 2014 at 7:07

[GoogleCodeExporter]

Huh, I thought I'd tried that case too, but I didn't work with your original 
example. I'll try and get back to it and do so, but I'm not sure I'll have time 
before Easter.

Original comment by kim.gras...@gmail.com on 16 Apr 2014 at 8:46

[GoogleCodeExporter]

Assuming you're doing:

  include-what-you-use -I. libtest\mytestclass.cpp

I think you may be seeing the effects of the precompiled header + macros again.

When I get rid of PCH and fix up the include dependencies of mytestclass.*, 
things appear to be working (even without the patch, actually, probably the 
presence of macros override the need to analyze static members.)

Attached example works for me with IWYU @HEAD if I just do:

  include-what-you-use testclass.cpp

Original comment by kim.gras...@gmail.com on 21 Apr 2014 at 7:32

Attachments:

• macro_static.zip

[GoogleCodeExporter]

Hmmm yes, but in your macro file you are include util.h, which is something 
that is done only on the cpp where the macro implementation is used (I mention 
that on the first post). The idea was to not include the class when not needed, 
only forward declare it.

Original comment by dpun...@gmail.com on 22 Apr 2014 at 3:59

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I just tried to include myutil on the macro file like you did and then I don't 
get any warning, but that would defeat the purpose of the forward declaration.

What do you get if you move [#include "util.h"] from util_macros.h to 
testclass.cpp ?

Original comment by dpun...@gmail.com on 22 Apr 2014 at 7:41

[GoogleCodeExporter]

It asks me to remove it from testclass.cpp, after which testclass.cpp fails to 
compile.

I'm not sure I see what you want to accomplish here.

The way I see it, the include belongs in util_macros.h pulling in util_macros.h 
will invariably mean you're pulling in util.h sooner or later. Or is it that 
you want headers including util_macros.h to not have to include util.h?

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

[GoogleCodeExporter]

> Or is it that you want headers including util_macros.h to not have to include 
util.h?

Exactly, because headers that are using the declare macros don't need that 
include, since the class myutil is forward declared. So in this case, 
mytestclass.h can live without including myutil.h, which will lead to 
recompiling less when myutil changes, if myutil.h is included in many files.

Original comment by dpun...@gmail.com on 22 Apr 2014 at 7:58

[GoogleCodeExporter]

Thanks for helping me understand.

Unfortunately, if I add a ZMyUtil forward declaration in util_macros.h and 
include util.h only in testclass.cpp, both Clang and MSVC fail to compile the 
file:

--
$ clang -fsyntax-only testclass.cpp
testclass.cpp:4:1: error: variable has incomplete type 'ZMyUtil'
MY_NAMESPACEUTIL_IMPL(ZMyTestClass)
^
./util_macros.h:11:24: note: expanded from macro 'MY_NAMESPACEUTIL_IMPL'
    ZMyUtil CLASSNAME::ms_myOtherClass;     \
                       ^
./util_macros.h:4:7: note: forward declaration of 'ZMyUtil'
class ZMyUtil;
      ^
1 error generated.
--

This make no sense to me, but it appears to be invalid... I'm guessing your 
original code is compilable, so can you help me tease out what I got wrong?

Thanks!

Original comment by kim.gras...@gmail.com on 23 Apr 2014 at 6:26

[GoogleCodeExporter]

That's very strange, I compiled your example just fine by moving util.h into 
testclass.cpp

I attach a rar with the setup with your code. Do you see any difference?

Here is the command line used and the report given by iwyu:

F:\tests\build\bin\Release\include-what-you-use.exe 
F:\tests\kimtest\kimtest\testclass.cpp -IF:\tests\kimtest\kimtest\

(F:/tests/kimtest/kimtest/testclass.h has correct #includes/fwd-decls)

F:/tests/kimtest/kimtest/testclass.cpp should add these lines:

F:/tests/kimtest/kimtest/testclass.cpp should remove these lines:
- #include "util.h"  // lines 2-2

The full include-list for F:/tests/kimtest/kimtest/testclass.cpp:
#include "testclass.h"
---

Original comment by dpun...@gmail.com on 23 Apr 2014 at 7:51

Attachments:

• kimtest.rar

[GoogleCodeExporter]

I was probably too tired to see something obvious yesterday, I now see exactly 
the same as you do.

There's something about the use location of types used in a macro that's not 
acting the way we'd expect here. All ZMyUtil uses are attributed to 
util_macros.h rather than testclass.cpp.

Part of me thinks that's correct -- the macro hides and re-exports ZMyUtil -- 
but I also see your point about forward declarations. IWYU has some (really 
hairy) special cases collectively called "author intent" (see 
https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse#Automatic_re-expo
rt:_typedefs), where it interprets a forward declaration as a signal that the 
caller is responsible for including the full type. This sounds like what you 
want, but I don't think it's implemented for macros, and I'm not sure what it 
would take to do so.

I'd like to get the nominal handling of static members in first, so let me make 
a full patch including test cases for that, and then maybe we can open a 
separate bug for this issue. I'll try and understand the causes better first, 
so we know what we're up against.

Thanks for helping with details!

Original comment by kim.gras...@gmail.com on 24 Apr 2014 at 8:04

[GoogleCodeExporter]

I see! The typedef example looks very similar to this case. We explicitely 
forward-declare the type, so we want everyone to add the include on their own 
if they ever want to use the implementation.

One thing I don't get is, if this is supposed to be supported already without 
macros, why doing it with macros is different? Aren't macros expanded as normal 
code when used? I don't know all the details, I may be going too far saying 
this, but I wanted to ask :)

Original comment by dpun...@gmail.com on 24 Apr 2014 at 8:12

[GoogleCodeExporter]

I don't know all the details either :-) A lot of these problems were already 
solved when we inherited the project.

I do know that the intersection of macros and proper C++ language is where it 
gets really hairy, because we employ different Clang hooks to inspect them 
(preprocessor callbacks and AST traversal respectively) and the macro support 
in the AST is not perfect.

Also, the author intent/automatic reexport feature is implemented on a 
case-by-case basis and spans over both AST traversal and IWYU analysis, which 
makes it more difficult than just detecting a use and acting on it.

I think it should be doable, I'm just not sure how, yet :-)

Original comment by kim.gras...@gmail.com on 25 Apr 2014 at 4:42

[GoogleCodeExporter]

Attached is a revised patch with a simple test for static data members, please 
take a look!

Original comment by kim.gras...@gmail.com on 26 Apr 2014 at 8:39

Attachments:

• 127r2.patch

[GoogleCodeExporter]

Looks good to me. The only thing is that usually there is an empty line between 
// Tests... comment and actual test. Though my claim is backed only by a 
"scientific" research of checking randomly 10 tests.

Original comment by vsap...@gmail.com on 27 Apr 2014 at 3:24

[GoogleCodeExporter]

Thanks! r538, and I added an extra blank line. I was probably afraid to add it 
after your earlier remarks :-)

Original comment by kim.gras...@gmail.com on 27 Apr 2014 at 7:34

[GoogleCodeExporter]

Nah, I dislike changes, not just empty lines :-). If there were already empty 
lines, I am for keeping them. If there were no empty lines, I am against adding 
new.

Original comment by vsap...@gmail.com on 27 Apr 2014 at 8:50

[GoogleCodeExporter]

This doesn't fix the macro case, right?

Original comment by dpun...@gmail.com on 28 Apr 2014 at 1:35

[GoogleCodeExporter]

No, not yet. I plan to continue looking at that, I don't think it's really 
related to static members as such, so I wanted to get that out of the way first.

Original comment by kim.gras...@gmail.com on 28 Apr 2014 at 2:50

[GoogleCodeExporter]

Awesome. Thank you Kim.

Original comment by dpun...@gmail.com on 28 Apr 2014 at 3:06